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 4 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
The `9.0.0` dependencies moved DLL files to the `bin` directory, so
that directory needs to be added to `PATH` via the `LIBPATH` setting.
@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.

On Windows, the DLL files are in the installation `bin` directory and
need to be removed before building.
@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.

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