-
Notifications
You must be signed in to change notification settings - Fork 82
[CMAKE] Make vc6 build process easier #1210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
540c25a
to
f270720
Compare
CMakePresets.json
Outdated
"name": "vc6-vars", | ||
"hidden": true, | ||
"environment": { | ||
"VS6_DIR": "$env{ProgramFiles(x86)}\\Microsoft Visual Studio", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it is installed here: "C:\Program Files (x86)\Microsoft Visual Studio 6.0"
So I suspect it will only work on select installs? What happens if this is not found? Will it error then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW that directory is not in line with our own guide: https://github.com/TheSuperHackers/GeneralsGameCode/wiki/build_with_msvc6 which expects C:\Program Files (x86)\Microsoft Visual Studio\
.
If you try to compile you'll get an error in the VS2022 output window: "The system cannot find the path specified."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it is installed here: "C:\Program Files (x86)\Microsoft Visual Studio 6.0"
So I suspect it will only work on select installs? What happens if this is not found? Will it error then?
I tried really hard to make it work with cmakelists or a toolchain file so that I could use things like conditional logic and more advanced features to make it find the correct path for the user if they don't have it in the standard location or would like to pass it as an environment variable but unfortunately environment variables set up that way don't get picked up by the compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a custom install. It would be good if it does not fail compiling for those that have custom installs. Maybe we can ask AI to give idea how to solve accomodating custom installs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a custom install. It would be good if it does not fail compiling for those that have custom installs. Maybe we can ask AI to give idea how to solve accomodating custom installs?
Best I can think of is that you edit the value of VS6_DIR locally for now. I've already tried asking chat gpt and it's not very helpful but you can try with other AI models
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a custom install. It would be good if it does not fail compiling for those that have custom installs. Maybe we can ask AI to give idea how to solve accomodating custom installs?
Actually it possibly does work if I do it in cmakelists.txt even before the toolchain file is called. Let me try that and I'll update it if it works. Then we don't even need a separate preset than the one for the CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm, it just seemed to work because of cache. Unfortunately even doing it as the first thing in the top level cmakelists.txt doesn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok now it should work correctly if you set the environment variables yourself like the CI does as long as you set all three, PATH, INCLUDE, and LIB. I had made a typo before. Forgot to add the $ for penv so it was replacing the values rather than prepending.
Now it seems to work correctly with the CI too so I removed the additional presets added the enviroment setup to the originals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what command line arguments would be used now to build with this? And how would this build from within Visual Studio 2022?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in visual studio you just click project>delete cache and reconfigure and then select "Windows 32bit VC6 Release" as the preset and then build>build all, or select a target and build just the target. For command line you can continue using your previous method and it will still work or do the same thing you do for building the c++20 presets.
Works fine for me. |
Added environment variable initialization to CMakePresets for vc6 It will no longer be necessary to setup a separate CMD session with vsvars32.bat just for VC6 builds. Switching to and from a vc6 build should now just be a matter of switching presets
@@ -12,6 +12,12 @@ | |||
"generator": "NMake Makefiles", | |||
"hidden": false, | |||
"binaryDir": "${sourceDir}/build/${presetName}", | |||
"environment": { | |||
"VS6_DIR": "$env{ProgramFiles(x86)}\\Microsoft Visual Studio", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hardcodes a path which doesn't work if its not at this path. I use a portable install that I have at an arbitrary path. I have to run the vcvars32.bat for it to make it work which isn't making my life easier. Even setting VS6_DIR doesn't fix it because this overwrites whatever I have set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you figure out how to make it work for arbitrary paths without setting up environment outside of cmake then feel free to post a pull request with the alternate implementation. Without this, the only option is to use vcvars32.bat. With this, that option still remains but additionally for users who have their install in the default path or are willing to relocate their install to the default path, this makes it much easier. Alternatively they could also just edit the value of VS6_DIR in cmake presets to match their arbitrary path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think editing the value in the CMakePresets.json is a behaviour we should even suggest, much less encourage or we'll end up with PRs inadvertently including changes to it. I'd rather this kind of enviroment specific config was provided in an example CMakeUserPresets.json instead that the user can copy to the root of their local repo copy and modify as appropriate as this file is already excluded by gitignore. This kind of user specific stuff is exactly what the user presets file is for.
This would give users 2 options....
- Run vcvars32.bat
- Copy our example to CMakeUserPresets.json to the root, modify as required. Use the custom preset and don't think about it again.
Added environment variable initialization to CMakePresets for vc6
It will no longer be necessary to setup a separate CMD session with vsvars32.bat just for VC6 builds.
Switching to and from a vc6 build should now just be a matter of switching presets