-
Notifications
You must be signed in to change notification settings - Fork 42
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
Trying to fix all of the builds #352
Conversation
What was the reason to include boost/filesystem.hpp? Unrelated, but it looks like the MacOS checks are never going to run because GitHub removed support for the version they want to run on… :( |
The reason to include filesystem.hpp (as far as I understand it) is that Visual Studio handles recursive includes differently than gcc does... I think. I think for gcc, if file 1 includes file 2 which includes file 3, then file 1 can reference names defined in file 3. I think for Visual Studio's compiler, file 1 needs to include file 3 directly to get those names. I could be wrong about why, but adding the includes definitely fixes the windows build. |
That doesn't make sense for reasoning… but adding them alongside operations.hpp may be redundant, I think? That is to say, if we're including filesystem.hpp it is quite likely redundant to also include filesystem/operations.hpp. |
If that's not the reason why Windows requires extra include statements, then the reason must be even more illogical and terrifying. |
This reverts commit 48fa461.
Trying to pin the dependencies on Windows and Mac brought only heartache, so I reverted all my commits that were trying to do that (at least for now). For some reason the visual studio builds failed this time, looks like with a network error when downloading packages? Trying to re-run those builds later should hopefully fix it because I think this commit is identical to the one that worked. |
proj/vs2017/vcpkg.json
Outdated
@@ -1,4 +1,5 @@ | |||
{ | |||
"builtin-baseline": "007aaced1a9d3245e28a2ba9395dca88ea890db1", |
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.
What is this…?
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.
That's a horrible thing that vcpkg requires if you want to specify an out-of-date version of a dependency. It's a commit hash for the vcpkg central repo. I put it in correctly and it still didn't work.
I made a redo PR of this, minus the changes that broke it after the one build passed |
For now, I can't figure out why, but my fork won't run the CI actions. So, I don't know if this is going to work, but I've tried adding a dependency to install-deps.bat and we'll see what happens.