-
Notifications
You must be signed in to change notification settings - Fork 86
JLD2 extension take 2 #597
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: master
Are you sure you want to change the base?
Conversation
I got the tests to pass. However, I'm not sure this is the best way to deal with the new JLD2 requirement. Do we add a JLD2 dependency directly to this package's setup file so that the downstream packages require no changes, or do we add JLD2 to every downstream package's test Project.toml? Also, @maleadt I removed the |
Removing TestEnv is fine if the alternative works.
In the past, we've opted for adding them to the test Project.toml, as running Pkg operations (mutating the active environment) during testing is not great, and may affect versions of other packages installed. It's an annoying way to handle this, but I don't think we can avoid it. |
Edit: I see now what is going on: the buildkite pipeline runs the tests of the downstream page (e.g. CUDA) and that package itself ends up including https://github.com/JuliaGPU/CUDA.jl/blob/8b6a2a06c8f9d247081d7c92d8f8a82ab68153d5/test/setup.jl#L12 which then triggers the "missing JLD2` dependency error. I guess without fundamentally reworking the test infrastructure (e.g. by pulling out the testsuit stuff into its own package), merging dependencies manually (as @christiangnrd ) is unavoidable. Perhaps a minor improvement over the current approach: rather than manually adding only for (uuid, pkg_info) in gpuarray_test_deps
Pkg.add(PackageSpec(name=pkg_info.name, uuid=uuid, version=pkg_info.version))
end |
This is for reverse-CI, executing e.g. CUDA.jl's tests, which import |
You beat me to it! :) See my edited comment above ☝️ |
I opened JuliaGPU/CUDA.jl#2792, JuliaGPU/OpenCL.jl#329, JuliaGPU/oneAPI.jl#507, and merged JuliaGPU/Metal.jl#606. Once merged we can rerun tests and then probably good to go. |
@lassepe I accidentally messed up your PR (#596) so I'm reopening this. I'll try to make it pass tests but if you get it fully working before then feel free to reopen and I'll close this one.
I apologize for the inconvenience.