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 f(ctx methods part of API #2952

Closed

Conversation

IanButterworth
Copy link
Member

As @KristofferC pointed out in #2938 (comment) the add(ctx.. etc entry points are considered internals.

BinaryBuilder needs to set add(Context(; julia_version), ...) for the julia version resolver stuff.

So it seems best to make that format formally part of the API, and pass them through the necessary processing steps.

This also distinguishes the API entry points from internals.

Fixes #2938

Todo:

  • Add tests for all the added API entrypoints

cc. @giordano

@IanButterworth IanButterworth marked this pull request as draft January 23, 2022 02:05
@IanButterworth IanButterworth force-pushed the ib/context_api branch 2 times, most recently from e74c757 to b778a8d Compare January 23, 2022 03:27
@KristofferC
Copy link
Member

BinaryBuilder needs to set add(Context(; julia_version), ...) for the julia version resolver stuff.

Why? They can just use Pkg.add("Package"; julia_version=v"1.6") for example.

@DilumAluthge
Copy link
Member

Making Context (and methods that accept it) part of the public API removes our ability to make breaking changes to it in the future.

If BinaryBuilder needs to use Pkg internals, that's totally fine. We should just make sure that BinaryBuilder CI includes a job that runs on Julia nightly, so that when internals change, we catch it sooner, and fix it.

But that doesn't mean we should make those internals part of the public API.

@IanButterworth
Copy link
Member Author

Why? They can just use Pkg.add("Package"; julia_version=v"1.6") for example.

I was under the impression specifying the context was necessary. Perhaps @giordano could expand/correct me.

@giordano
Copy link
Contributor

giordano commented Jan 23, 2022

This is the code: https://github.com/JuliaPackaging/BinaryBuilderBase.jl/blob/74fd093d3ab33f9410a64cd42b28662654a014fe/src/Prefix.jl#L535-L665. We use ctx for other things in the function, not just Pkg.add, if we pass julia_version only to Pkg.add then julia_version isn't retained in ctx. If the function can be improved, please do.

I tend to agree with the sentiment that freezing the internal functions just for one package that happens to hook into them may not be a great idea, but, well, we do need to do weird things with package resolution. I opened JuliaPackaging/BinaryBuilderBase.jl#129 almost one year ago exactly to try and catch failures in nightly early, but there were already failures and took forever to work around them (in the meantime other stuff broke), asked for help in #pkg-dev on Slack several times, didn't get much. @staticfloat recently managed to rework BinaryBuilderBase.setup_dependencies to be compatible with v1.7+ (but now it doesn't work anymore in v1.6, fine, I don't care much about supporting older versions). However now there is the issue with LibOSXUnwind_jll which isn't a stdlib any longer and apparently this throws package installation out of the window (#2942 and #2930). It's unfortunate that that happened, but, well, it did happen and I guess there is no way back, but it doesn't change the fact we do need to be able to install libjulia_jll for v1.6, that's needed for all packages using CxxWrap, which at the moment are stuck. So I'd appreciate very much some help with fixing either #2942 or #2930 when installing libjulia_jll in the julia_version=v"1.6.0" context. Also yet another workaround in BB would be ok, but at the moment I have zero clue of what to do.

@giordano
Copy link
Contributor

They can just use Pkg.add("Package"; julia_version=v"1.6") for example.

BTW, no, we can't: #2930

@KristofferC
Copy link
Member

if we pass julia_version only to Pkg.add then julia_version isn't retained in ctx.

That sounds like a bug.

I still don't know personally what exactly the whole julia_version business is about and what it is supposed to do when it is passed. If it is to simulate a resolve on a different julia version you need to bundle the project file for every stdlib for every julia version (alt have some sort of mini stdlib registry that encodes the same information).

@IanButterworth
Copy link
Member Author

Closing as unpopular.

Although, the fact tests failed here may point to an issue in existing tests, as AFAICT this shouldn't be breaking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants