search

beacon-biosignals / julia_pod

k8s native julia development
MIT License
10 stars 3 forks source link

Remove non-pinned packages more thoroughly #9

Closed ericphanson closed 3 years ago

ericphanson commented 3 years ago

This is easier explained with an example. Let's say I have AWSS3.jl and AWS.jl in my Project.toml, and I was working on AWS.jl so I un-pinned it so that I don't build it into my sysimage, because I want to be able to change versions (e.g. switch to my branch) without rebuilding it. Currently, we would rm AWS.jl from the Project before building the sysimage. But we would not rm AWSS3, which means that AWSS3 and all of its dependencies including AWS.jl would get baked into the sysimage. That means I would not be able to change versions of AWS.jl despite unpinning it.

Here, we use a Pkg command to remove the packages everywhere in the Manifest. That means if any package depends on a removed package, it is removed too. So in the example, we would not build either AWSS3 or AWS into the sysimage; they would both be removed since AWS.jl is not pinned.

That means if you do not pin something very low in the stack, you might end up removing most things from your sysimage. But that's basically the cost of editing something low in the stack I think; it seems unavoidable.

ericphanson commented 3 years ago

Ah, turns out this removes non-pinned stdlibs too, and anything depending on them. Hm..

kolia commented 3 years ago

Yeah, this does assume that users typically pin a maximal set of dependencies. If you forget some, it may rm a whole bunch of others. But I guess that's ok.

ericphanson commented 3 years ago

Yeah, I think that's a bit unfortunate. But I think maybe it's worse if there's confusing bugs over not realizing that a package got baked into the sysimage and you don't get why it's not seeing the new version.

ericphanson commented 3 years ago

I updated the logic a bit: now a package is set to be removed (recursively as discussed) if:

and to allow the "everything is pinned" scenario. (Pkg.rm errors if you give it an empty list).

kolia commented 3 years ago

Conversely, I've been wondering whether we should pin Manifest deps that are transitive dependencies of a pinned package. Right now without that, your sysimage has these transitive dependencies baked in, but Pkg doesn't know that and will happily think it's for example upgrading one of these when in fact it is fixed in sysimage stone.

The reason I haven't done that is that it doesn't seem too polite to modify the user's set of pinned packages for them. Could throw an error and provide some convenience function for pinning transitive deps, but that's not great either.

Also not sure how much trouble not pinning transitive deps this is likely to cause in practice, I suspect not much. All of this really needs to be fixed in Pkg, not here...

ericphanson commented 3 years ago

Yeah... I don't know what we should do here with respect to that.

@KristofferC has suggested on Slack that an upstream fix would be for PackageCompiler to record the versions of all the packages baked into the sysimage, and to teach Pkg.jl to read that and treat those packages as pinned.

That way Pkg wouldn't lie about upgrades and such. That would be pretty nice!