JuliaObjects / Accessors.jl

Update immutable data
Other
175 stars 19 forks source link

Make Dates and Test weak dependencies #168

Closed devmotion closed 3 weeks ago

devmotion commented 1 month ago

LinearAlgebra is kept as a direct dependency until https://github.com/JuliaObjects/ConstructionBase.jl/pull/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 https://github.com/JuliaObjects/ConstructionBase.jl/pull/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).

aplavin commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.

devmotion commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.