Skip to content
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

Update to Dependencies 9.0.0 #1434

Open
wants to merge 2 commits into
base: RB-10.5
Choose a base branch
from

Conversation

ericmehl
Copy link
Collaborator

This updates the Cortex build to use Visual Studio 2022 / MSVC 17.x / Runtime library 14.3.

This should be built in conjunction with Gaffer dependencies from the 9_maintenance_vs2022 branch. I'm targeting this to main because I think it constitutes a breaking change that should put it in a... Cortex 10.6 release?

The dependencies on 9_maintenance_vs2022 has a patch equivalent to this commit, so I don't think there's a hurry to get it merged into a release at the moment.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Cortex project's prevailing coding style and conventions.

@johnhaddon
Copy link
Member

scons: warning: MSVC version '14.3' was not found.

I think we need to update the CI config to use the new dependencies, and also install the right MSVC stuff on the runner.

I'm targeting this to main because I think it constitutes a breaking change that should put it in a... Cortex 10.6 release?

Personally I'm prepared to play fast and loose with this. I'm betting that outside of Image Engine, Cortex just exists to serve Gaffer, and it'll be fine to change the dependencies we include in the release packages. But we probably do want to delay this until Gaffer 1.4 is in patch maintenance mode (since we're likely to need new Cortex releases for it beforehand).

But this does raise the question : when will we ever change Cortex major version? There are lots of little improvements I'd love to make that would mean a new major version, but the friction involved in synchronising it all with Gaffer tends to mean that I don't bother. For me the solution would be a much slimmed version developed directly in the Gaffer repo, but that has its own problems...

@johnhaddon
Copy link
Member

But we probably do want to delay this until Gaffer 1.4 is in patch maintenance mode

Alternatively, we provide two CI variants, just as we are doing for Linux already?

@johnhaddon
Copy link
Member

@ericmehl, I've merged #1443, and Gaffer 1.5 is released, so I think you should have a clear run at this now. Would be great to get it over the line, as we have some Cortex updates we'd like to get into future Gaffers.

@ericmehl ericmehl changed the base branch from main to RB-10.5 November 11, 2024 17:57
@ericmehl ericmehl force-pushed the visualStudio2022 branch 2 times, most recently from 3884a77 to 92bee1a Compare November 12, 2024 16:54
@danieldresser-ie
Copy link
Contributor

In an effort to debug this test failure, I've created #1444, where I replace the constructor for MeshSplitter with something that just throws an exception. This is still resulting in the same output. I therefore conclude that somehow this Windows change is resulting in not actually finding the freshly built Cortex - because this is using Gaffer dependencies to get the dependencies, it must be finding the Cortex binaries from Gaffer dependencies, and running the new Cortex tests against that version of Cortex, not the current code that is being compiled.

@ericmehl ericmehl changed the title SCons : Windows build on VS 2022 Update to Dependencies 9.0.0 Nov 14, 2024
@ericmehl
Copy link
Collaborator Author

In ec23650 I fixed the Windows builds by removing the binaries from the dependencies package. And in 1fbd16f I have SCons moving the DLL files to the bin directory. It's a bit awkward, but I didn't see any clever SCons tricks to separate the .dll and .lib files - SCons handles those as a unit.

Ready for review.

@johnhaddon
Copy link
Member

In 1fbd16f I have SCons moving the DLL files to the bin directory. It's a bit awkward, but I didn't see any clever SCons tricks to separate the .dll and .lib files - SCons handles those as a unit.

I can't help but feel that we're chasing our own tails here. I don't understand the benefit of having the DLLs in the bin. It seems like it was a bit of a faff to put them there in the first place. It separates them from the .lib files that they are most closely related to. It clutters up the bin directory so it's harder to see the executables, making the gaffer.cmd wrapper harder to find for a demographic who are far less comfortable with that sort of thing already. And now we're adding complexity to the SConstruct to maintain it all.

Admittedly, I am completely ignorant of Windows conventions and am coming at this from a Linux perspective. But unless there is a compelling reason that trumps all of the above, I think keeping the libs in lib would be the simpler solution overall. Can we consider it?

@ericmehl
Copy link
Collaborator Author

It's certainly something worth considering, I agree that it's annoying to have to manually shuffle around the DLLs and clutter up the bin directory. My case for enduring the faff rests on a few points :

  • The Python search for binaries needed by modules gives first priority to the directory of the Python executable. I'd say it's equally faffy to do the add_dll_directory bit at startup that we have to do, and we've had a few issues with users not knowing they had to import Gaffer before importing much anything else in order to find the DLL directory. I know it would actually spread the annoying shuffling needed, but we should be able to eliminate that if we extend this approach to Gaffer as well.
  • The various dependency packages are inconsistent with which directory they install DLL files to. Some go to lib and some to bin. We could consolidate almost all into lib, but at the least python*.dll has to be alongside python.exe for it to be found. So we could never consolidate 100% into lib.
  • Looking at how other software organizes things, all that I have, open-source and closed, have DLL files in the same directory as the binary, including a few VFX packages that I'm guessing started their lives on Linux. (Most actually have the DLLs and executables in their equivalent of $GAFFER_ROOT rather than a bin subdirectory. But I'm not advocating for that level of chaos.)
  • I think there's a minor case to be made that from most users' point of view, the DLLs are more closely related to the executable than to the import libraries. They "make the program work" whereas the .lib files are for building c++ libraries to extend functionality and aren't necessary.

I'll rest my 💼 on those points. If you don't think that balances out the drawbacks, I can pop off the DLL shuffle commit.

@johnhaddon
Copy link
Member

Summary of various offline discussions :

  • We're more concerned about the ergonomics of the layout than we are following Windows conventions to the letter. Or at least I am, and I'm tyrannical enough to push it through.
  • There are three ergonomic concerns :
    • bin staying clean so the only things in there are executables, and you can see what's you might want to use without difficulty.
    • Not having extra complexity in our build processes (like that introduced in this PR).
    • Being able to import modules in Python without jumping through add_dll_directory() hoops.
  • Of those, the latter was our big sticking point. But we've learned that we can use a simple sitecustomize file to set call add_dll_directory immediately on Python startup, whether or not Python is started directly or via our wrapper. That means we can have our cake and eat it.

So, the new plan is this :

  1. We'll make a new Cortex release without merging this PR.
  2. We'll make a new GafferHQ/dependencies release with an updated Cortex. This will also move the Cortex libs back to lib.
  3. Then we'll simplify this PR to use the new GaffferHQ/dependencies release, keeping the Cortex libs in lib.
  4. Then in a later GafferHQ/dependencies release, we'll move all DLLs into lib and use the sitecustomize gambit for a glorious future.

@ericmehl
Copy link
Collaborator Author

ericmehl commented Nov 20, 2024

I've dropped the last three commits to go back to the lib world, and I updated the dependencies download to 9.1.0. I pushed those up even though they will fail on Linux for now. When the Linux dependencies are up, we can rerun those CI builds.

@ericmehl ericmehl force-pushed the visualStudio2022 branch 2 times, most recently from bfa0d1b to 9f20bfd Compare November 20, 2024 20:24
This is needed for the tests to locate the DLL binaries that are
currently in both `bin` and `lib` subdirectories. When all DLLs are
moved to `lib`, this can be reverted.
@ericmehl
Copy link
Collaborator Author

I was a bit overzealous with dropping commits after moving the DLLs. 3c617db is still needed for the tests to find the rest of the DLLs that haven't moved to lib yet. When that move is done, we can take the bin directory back out of the lib search path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants