JuliaApproximation / DomainSets.jl

A Julia package for describing domains as continuous sets of elements
MIT License
72 stars 12 forks source link

are there conflicting versions of isapprox in IntervalSets? #127

Closed a2ray closed 1 year ago

a2ray commented 1 year ago

For my package HiQGA.jl, I keep getting this new error even on reverting to stable versions

[ Info: Precompiling HiQGA [01574e87-63b6-4466-925a-334cf7e21b6b]
WARNING: Method definition isapprox(IntervalSets.AbstractInterval{T} where T, IntervalSets.AbstractInterval{T} where T) in module IntervalSets at /blah/.julia/packages/IntervalSets/voJZA/src/IntervalSets.jl:146 overwritten in module DomainSets at /blah/.julia/packages/DomainSets/aafhp/src/domains/interval.jl:52.
  ** incremental compilation may be fatally broken for this module **

Could this perhaps be related to e3185fe73c7b67fcede158389499cf521a9d5522 ? I do not explictly use DomainSets or IntervalSets in my code.

daanhb commented 1 year ago

Thanks for reporting. The issue arises because conflicting versions of isapprox have been defined. This isn't a problem in DomainSets v0.6 anymore, but I guess in your case you have no control over that as the packages you depend on would have to upgrade DomainSets.

daanhb commented 1 year ago

I suspect this will happen to any user who combines IntervalSets 0.7.4 with DomainSets 0.5.x. The problem arose because IntervalSets defined isapprox for intervals in a way that was a breaking change for DomainSets, which led to DomainSets releasing v0.6. But IntervalSets did not have a similar version increase. That leaves this warning for anyone not using DomainSets v0.6, and it will remain that way until everybody upgrades.

Any thoughts on remedying this situation? @dlfivefifty @jishnub @hyrodium

dlfivefifty commented 1 year ago

Revert the change in IntervalSets, tag, Revert the revert, tag again as a breaking chagne

hyrodium commented 1 year ago

I'm understanding this was because of the type piracy in DomainSets.jl. Adding isapprox in IntervalSets.jl is just a new feature, so we don't need to update the minor version of IntervalSets.jl. If one wants to avoid this problem, just add IntervalSets.jl by the following command.

]add IntervalSets@v0.7.3
a2ray commented 1 year ago

Yes, but what about those who depend on IntervalSets but are not directly adding it in their packages? A breaking change release would be better in this case without requiring that I track down my entire package tree, no?

jishnub commented 1 year ago

Maybe we can have a 0.5.x release in DomainSets that sets an upper cutoff on IntervalSets to 0.7.3? This isn't a full fix, as the resolver may downgrade DomainSets and upgrade IntervalSets, but it'll address the problem somewhat. Ideally we should create a PR in the general registry to limit the compat bounds on IntervalSets as well for all DomainSets 0.5.x releases. If all versions of 0.5.x are compatible with IntervalSets 0.7.3 at most, this problem won't arise

daanhb commented 1 year ago

Maybe we can have a 0.5.x release in DomainSets that sets an upper cutoff on IntervalSets to 0.7.3? This isn't a full fix, as the resolver may downgrade DomainSets and upgrade IntervalSets, but it'll address the problem somewhat. Ideally we should create a PR in the general registry to limit the compat bounds on IntervalSets as well for all DomainSets 0.5.x releases. If all versions of 0.5.x are compatible with IntervalSets 0.7.3 at most, this problem won't arise

This looks like the simpler option - can that be done with the help of JuliaRegistrator or is this manual work? I have no experience with creating out-of-order releases.

jishnub commented 1 year ago

We create a separate branch from the last 0.5.x release and push it to the repo, commit the changes to that branch, and tag that commit using JuliaRegistrator. The branch will need to co-exist with master, and will be a backport branch thereafter (may be neglected if no updates are necessary).

daanhb commented 1 year ago

That seems to have worked and 0.5.15 is out, though I'm noticing some glitches. The Diff since 0.5.14 in the release page is fine, but the list of closed issues and merged pull requests on that page is not correct, as it shows commits not included into this release.

jishnub commented 1 year ago

Yes, that's a bit unfortunate. Should we also update the compat limits in the general registry? This has to be manual PR. Perhaps all old versions compatible with IntervalSets v0.7 need to be capped to 0.7.3

BTW @a2ray does this resolve your issue? Could you try a package upgrade and check again?

j-fu commented 1 year ago

seen this for a while as well, after upgrade this is gone.

j-fu commented 1 year ago

seen this for a while as well, after upgrade this is gone.

a2ray commented 1 year ago

BTW @a2ray does this resolve your issue? Could you try a package upgrade and check again?

Yes it does, on upgrading HiQGA I get

⌅ [5b8099bc] ↑ DomainSets v0.5.14 ⇒ v0.5.15

without the old incremental compilation error. Is fixed now, many thanks!

dlfivefifty commented 1 year ago

I think in retrospect we should of considered the removal of isapprox here and the changing behaviour a "bug fix" so it would have been a patch.....