Upper bound Reexport.jl to fix compilation issue

[torfjelde]

simonster/Reexport.jl#33 (comment)

[devmotion]

Another fix would be to rename Turing.Core to something else I assume 😛

[torfjelde]

Another fix would be to rename Turing.Core to something else I assume

But isn't this a bug on the end of Reexport.jl though? In the sense that this is allowed by the language.

[devmotion]

Yes, I guess so. I assume it would be sufficient to interpolate Core in the expression.

[torfjelde]

Yes, I guess so. I assume it would be sufficient to interpolate Core in the expression.

In which expression? Core is not present in a @reexport if that's what you mean.

[devmotion]

In the following way: simonster/Reexport.jl#35

[devmotion]

Maybe we can skip this PR if the PR to Reexport is merged and released without too much delay. The problem with such bounds in Turing in general is that it is not clear that it will help users without manual intervention. If they just ] up, the Pkg resolver might decide that it's best to not downgrade Reexport and they would not end up with the new version of Turing. To fix the issue they should therefore perform a more explicit ] add Turing@0.17.2 - but then they could equally well just run an ] add Reexport@1.2.0 🤷

[torfjelde]

In the following way

Ah gotcha!:)

Maybe we can skip this PR if the PR to Reexport is merged and released without too much delay.

But even then we still need to ensure that we're not compat with 1.2.1, right? So yes but we still need a PR to fix bounds.

the Pkg resolver might decide that it's best to not downgrade Reexport and they would not end up with the new version of Turing.

Sure, but the big thing here is "might". Most will have the same bound for Reexport as we currently have, i.e. 1. This will then work nicely with this version bound.

[coveralls]

Pull Request Test Coverage Report for Build 1162456986

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 80.071%

---

Totals
Change from base Build 1131328898:	0.0%
Covered Lines:	1125
Relevant Lines:	1405

---

💛 - Coveralls

[codecov]

Codecov Report

Merging #1686 (ab84663) into master (b277b01) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1686   +/-   ##
=======================================
  Coverage   80.07%   80.07%           
=======================================
  Files          23       23           
  Lines        1405     1405           
=======================================
  Hits         1125     1125           
  Misses        280      280           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b277b01...ab84663. Read the comment docs.

[devmotion]

The main point is: this PR does not help if users/packages/etc. don't end up with the new Turing version. And if they already use Reexport@1.2.1 since e.g. they updated their environment, then it is not guaranteed that they end up with the new version since Reexport would have to be downgraded.

The Reexport bugfix release would solve these issues since the problem would be solved by just running ] up without downgrading any packages.

The only way to ensure that Reexport@1.2.1 never shows up in any installations is to change the compatibilities in the general registry. Even this PR won't guarantee that some users don't end up with Reexport@1.2.1 since older versions of Turing are compatible with it (at least officially).

[torfjelde]

The main point is: this PR does not help if users/packages/etc. don't end up with the new Turing version. And if they already use Reexport@1.2.1 since e.g. they updated their environment, then it is not guaranteed that they end up with the new version since Reexport would have to be downgraded.

Hmm, how would the following end up with you did ] up?

Turing = "0.17"
Reexport = "1"

Surely Pkg gives precedence to the package which explicitly restricts the other, no? I.e. it results in the most up-to-date Turing version.

[devmotion]

I don't know, Pkg and its decisions are still a mystery to me 😄 I just spent almost an hour trying to debug a Pkg weirdness and I still don't know what's going on. My gut feeling is that upper bounding package versions in new releases while they are unbounded in previous ones is no golden bullet. For instance, people opened issues all the time about some compilation error that was fixed in recent versions but somehow they all ended up with outdated versions of Turing and actually had to force the update to a more recent one. In the end I just fixed all compatibility bounds in the registry and it seems this worked 😄

That being said, I am not generally opposed to this PR but I think the better solution would be a bugfix release of Reexport 🙂

[torfjelde]

Aaah, okay 😅 Yeah I kind of just assumed it would do the above. And the isuse you're talking about sounds awful..

But I'll merge for now since it's holding up PRs in other packages 👍

[ararslan]

FYI, Reexport v1.2.2 has been released which fixes the issue with Turing, and v1.2.1 has been yanked from General. Since Turing requires Julia 1.3, it should be safe to revert to a compat entry of Reexport = "1"

[devmotion]

Thanks, I already opened a PR 🙂