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

Update SDPA to new libcxxwrap / libjulia #2327

Merged
merged 1 commit into from
Jan 2, 2021

Conversation

fingolfin
Copy link
Member

Make some progress towards issue #2160

Should not be merged before @ViralBShah had a chance to review and comment. Also, there are some questions to discuss:

  1. Which version to use for this JLL? It really shouldn't be the same as right now, as the list of dependencies changes, and that requires a new version.
  2. Which Julia versions should this JLL be made compatible with? Right now, this is built for Julia 1.5.3 or later, and thus only usable in Julia >=1.5. But we could make versions supporting 1.4 or even 1.3 if desired. But that's extra work and complexity, so I first would like to get this version working, and also hear explicitly that Julia 1.3/1.4 support is wanted, before spending time on it.
  3. The two first points are actually somewhat related: in order to support multiple Julia versions, the trick we used in other JLLs is to use the patch level of the version string to indicate the Julia version; that would suggest to reversion this package to something like 0.3.X where X is equal to the one in the Julia version 1.X. However, then packages using this JLL may need to be updated.

using BinaryBuilder, Pkg

julia_version = v"1.5.3"

name = "SDPA"
version = v"7.3.8"
Copy link
Member Author

@fingolfin fingolfin Dec 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must change; but then it can't match the upstream version anymore. One pattern we used in other packages is to rename version to upstream_version and then use another made-up version for the JLL (in long discussions on Slack it was basically agreed that's decoupling the JLL version from the upstream version is the only long-term viable option anyway).

E.g. 700.300.800 could be used, or something completely else (it depends also on whether upstream follows semver or not; if not, then something like 703.800.0 might be better (I should write the rationale for these suggestions somewhere... later...)

Copy link
Member

@ViralBShah ViralBShah Dec 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just call it 7.3.9 and when upstream releases 7.3.9, just push a new BB version? Ugly but temporary.

Copy link
Contributor

@ericphanson ericphanson Dec 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember seeing there was a long discussion in #binarybuilder (and maybe a discussion in the Pkg triage call?) about how to version JLLs. I don’t actually know the outcome though, maybe @staticfloat or @giordano knows?

Copy link
Member Author

@fingolfin fingolfin Dec 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the discussion ended with "yup, we need to decouple JLL and upstream versions, for many good reasons, e.g.:

  1. upstream does not follow semver
  2. desire to package pre-release snapshots (e.g. via GitSource) -- note that using semver prerelease versions like 1.0.0-pre123 is not an option because the registry does not accept those (or at least that's the plan)
  3. the JLL needs updates that require a new version, even though the upstream version did not change).

What was not specific is how to version JLLs then. In one extreme, one can use arbitrary versions for the JLL that have no relation to the upstream version, and maintain a table somewhere (say, as a source code comment) which states how to map between JLL and upstream versions.

But that's rather annoying and not friendly towards humans. So for some of the JLLs I am involved with, we ended up with the following schemes, which leave plenty wiggle room to deal with the issues I listed above:

  • map X.Y.Z to X00.Y00.Z00 (i.e. multiply each component by 100). Useful if upstream (mostly) follows semver
  • map X.Y.Z to X0Y.Z00.0 -- useful for upstream packages where a minor version change routinely indicates major (ABI breaking) changes; or also for the situation we are looking at here, where the patch level could be (ab)used to disambiguate build for different Julia versions.

Some tricks one can pull of with this kinda of scheme:

  • If 1.2.3 breaks ABI compatibility with 1.2.2, then instead of mapping it to 100.200.300, just map it to 101.200.300.
  • to package a prerelease of 2.0.0, map it to 190.0.0 (or 199.0.0, but I prefer to always leave some wiggle room for major version changes; however, in our packages, we use ~ compat bounds for the JLLs anyway, so I could use 199.0.0 and if there is a new snapshot with breaking changes, I can call it 199.1.0
  • support different binaries for Julia 1.3, 1.4, 1.X... by inserting the "X" into the patch level

Of course there are endless variations to this scheme, and what fits also depends a bit on how the upstream project uses versions, and what other needs you have. My recommendation is to explicitly document the mapping with a source code comment, though -- grep for The version of this JLL is decoupled in Yggdrasil to see how I and my colleagues have handled it for our JLLs so far.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr: If this was my JLL, I'd change its version to 700.300.800, resp. (if you want to support Julia 1.3 and 1.4, too -- I haven't seen a comment stating your stance on this yet?) to 700.300.803 (for Julia 1.3), 700.300.804 (for Julia 1.4), 700.300.805 (for Julia 1.5 and later)

Copy link
Contributor

@ericphanson ericphanson Dec 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the explanation! To me, an “expansion” like 700.300.800 makes sense as a way to handle it. That seems easier to use than a lookup table and less confusing than calling it 7.3.9.

I’m only supporting Julia 1.5+ in my downstream SDPAFamily.jl, and it looks like @blegat’s SDPA.jl is doing the same. JuliaHub tells me we are the only General registry SDPA_jll users, so I don’t think it’s worth extra work to support earlier Julia versions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, good, I've now updated the version and added a comment explaining it for "future generations" ;-).

S/SDPA/build_tarballs.jl Outdated Show resolved Hide resolved
S/SDPA/build_tarballs.jl Outdated Show resolved Hide resolved
@ViralBShah
Copy link
Member

Should we merge?

@giordano giordano merged commit f062e26 into JuliaPackaging:master Jan 2, 2021
@fingolfin fingolfin deleted the mh/SDPA branch January 2, 2021 14:19
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.

4 participants