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

Make Dates and Test weak dependencies #168

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

devmotion
Copy link
Contributor

@devmotion devmotion commented Aug 29, 2024

LinearAlgebra is kept as a direct dependency until JuliaObjects/ConstructionBase.jl#95 is released: Without that PR, LinearAlgebra is a transitive dependency of Accessors and hence a LinearAlgebra extension of Accessors is not useful (and will even lead to warnings).

Edit: I think even if JuliaObjects/ConstructionBase.jl#95 is released before this PR is merged it would be beneficial to use a two step procedure and only make LinearAlgebra a weak dependency in a separate release: It requires to bump the ConstructionBase compat entry but due to the change in ConstructionBase 1.5.7 quite a few downstream packages and users might not be able to easily upgrade to a newer ConstructionBase version. By splitting the switch to weak dependencies over two releases, these users could at least benefit from the change in this PR (which does not require a new version of ConstructionBase).

@devmotion devmotion changed the title Make Dates and Tests weak dependencies Make Dates and Test weak dependencies Aug 29, 2024
@aplavin
Copy link
Member

aplavin commented Sep 13, 2024

This PR indeed doesn't seem to cause issues within Accessors tests. Of course, cannot be sure about more complicated environments, given the stdlib weakdep fragility.
But I tried to enable all weakdeps (this PR + linearalgebra), and then one gets that "big scary warning" even when running ]test for Accessors.
Not sure what exactly is happening, is it the pure number of stdlib extensions or some complex interaction? But that doesn't instill the confidence in the safety of such changes...

@devmotion
Copy link
Contributor Author

This PR indeed doesn't seem to cause issues within Accessors tests.

That's good, it's supposed to be a fix 🙂

But I tried to enable all weakdeps (this PR + linearalgebra), and then one gets that "big scary warning" even when running ]test for Accessors.

I can't say much without knowing the packages and their versions that were used when you ran Pkg.test. But I'm very certain that one of the (test) dependencies depends on LinearAlgebra, which then again created a cycle. Every problem and warning I've encountered so far is caused by this problem 🙂 Maybe you tested with an older version of ConstructionBase?

@aplavin
Copy link
Member

aplavin commented Sep 13, 2024

I can't say much without knowing the packages and their versions that were used when you ran Pkg.test.

Most recent versions of everything, started from scratch.

But I'm very certain that one of the (test) dependencies depends on LinearAlgebra

Sure, it's highly likely for some test dependency to depend on LinearAlgebra / Dates (how would you test these extensions otherwise?). And 100% some test dep depends on Test :)
The same applies to actual environments users have in practice.
But this cannot be the full reason for this warning, can it? The whole purpose of weakdeps is to load whenever some other package loads the "triggering" package.

That's good, it's supposed to be a fix 🙂

Having an extra dependency is not a bug, so removing it cannot be a "fix". It's strictly an optimization, and a pretty minor one for stdlibs.

@aplavin aplavin mentioned this pull request Sep 13, 2024
@devmotion
Copy link
Contributor Author

Having an extra dependency is not a bug, so removing it cannot be a "fix". It's strictly an optimization, and a pretty minor one for stdlibs.

Currently, Accessors breaks/holds back large parts of the ecosystem and this is resolved by the PR. I'd consider that a fix.

@aplavin
Copy link
Member

aplavin commented Sep 13, 2024

Accessors breaks

Having a dependency on a registered package, and even more so on an stdlib, is not "breaking" anything. Packages are created for others to depend on them :)

@devmotion
Copy link
Contributor Author

Basically every package tries to move Test utilities to an extension and considers a dependency on Test a bug. But downstream packages that depend on Accessors can't fix these issues as long as Accessors keeps enforcing an indirect dependency on Test.

@aplavin
Copy link
Member

aplavin commented Sep 13, 2024

Basically every package tries to move Test utilities to an extension and considers a dependency on Test a bug.

That's an ... unusual understanding of the word "bug" :)

Anyway, let's merge and see how it goes.

@aplavin aplavin merged commit d8a9314 into JuliaObjects:master Sep 13, 2024
6 of 10 checks passed
@devmotion devmotion deleted the dw/weakdeps branch September 13, 2024 14:48
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.

2 participants