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

Implement option to yank/blacklist a release. #726

Merged
merged 1 commit into from
Dec 4, 2018
Merged

Implement option to yank/blacklist a release. #726

merged 1 commit into from
Dec 4, 2018

Conversation

fredrikekre
Copy link
Member

@fredrikekre fredrikekre commented Sep 7, 2018

Implement support for yanking versions in a package registry.


Very WIP, but this allows us to blacklist releases on the registry level. They can not be installed, but they can still be used, and you can instantiate a Project.toml + Manifest.toml with it, so it should not break working code. Here https://github.com/fredrikekre/Registry/blob/97919d39c95a3d917ea3b69e78aad68c7c02bf7c/I/ImportMacros/Versions.toml I have blacklisted version 0.2.0 of ImportMacros, and consequently:

(v1) pkg> add ImportMacros
  Updating registry at `~/.julia/registries/General`
  Updating git-repo `https://github.com/JuliaRegistries/General.git`
  Updating registry at `~/.julia/registries/Registry`
  Updating git-repo `https://github.com/fredrikekre/Registry.git`
 Resolving package versions...
  Updating `~/.julia/environments/v1/Project.toml`
  [13b54a02] + ImportMacros v0.1.0
  Updating `~/.julia/environments/v1/Manifest.toml`
  [13b54a02] + ImportMacros v0.1.0

installs version 0.1.0. But we can instantiate a Manifest.toml with version 0.2.0 in it:

(v1) pkg> st
    Status `~/.julia/environments/v1/Project.toml`
→ [13b54a02] ImportMacros v0.2.0
┌ Warning: Some packages (indicated with a red arrow) are not downloaded, use `instantiate` to instantiate the current environment
└ @ Pkg.Display ~/dev/Pkg/src/Display.jl:190

(v1) pkg> instantiate
  Updating registry at `~/.julia/registries/General`
  Updating git-repo `https://github.com/JuliaRegistries/General.git`
  Updating registry at `~/.julia/registries/Registry`
  Updating git-repo `https://github.com/fredrikekre/Registry.git`
 Installed ImportMacros ─ v0.2.0

fix #355

@fredrikekre
Copy link
Member Author

fredrikekre commented Sep 7, 2018

Some questions:
Is Versions.toml a good place for this? Feels like it, but perhaps it is better to have it formatted like

["0.1.0"]
git-tree-sha1 = "..."

["0.2.0"]
git-tree-sha1 = "..."
yanked = true

which is also backward compatible.

  1. What should happen on up? Currently ImportMacros will be downgraded to 0.1.0.

@00vareladavid
Copy link
Contributor

up's behavior seems reasonable to me. What would happen on add ImportMacros@0.2.0?

@KristofferC
Copy link
Sponsor Member

Exactly the same as if the yanked version was not in the registry?

@fredrikekre
Copy link
Member Author

What should happen if A depends on B@v0.2 and B@v0.2 is yanked?

@KristofferC
Copy link
Sponsor Member

Probably wouldn't allow yanking that release in that case.

@fredrikekre
Copy link
Member Author

So that should be checked on registry CI then.

@simonbyrne
Copy link
Contributor

you can instantiate a Project.toml + Manifest.toml with it, so it should not break working code

In some cases, wouldn't you want to break the code if it has been blacklisted (i.e. if the code was malicious?)

@fredrikekre
Copy link
Member Author

Triage: In favor, but recorded in the registry as #726 (comment) rather than fredrikekre/Registry@97919d3

@fredrikekre
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Nov 27, 2018
@fredrikekre fredrikekre changed the title [WIP] Implement option to yank/blacklist a release. Implement option to yank/blacklist a release. Nov 27, 2018
@fredrikekre
Copy link
Member Author

Should be good to go now, but would be good if someone could review the calls to load_versions and see if I got the include_yanked = (true|false) correctly.

@bors
Copy link
Contributor

bors bot commented Nov 28, 2018

try

Build failed

@fredrikekre
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Nov 28, 2018
@bors
Copy link
Contributor

bors bot commented Nov 28, 2018

@@ -77,7 +80,7 @@ end
function set_maximum_version_registry!(env::EnvCache, pkg::PackageSpec)
pkgversions = Set{VersionNumber}()
for path in registered_paths(env, pkg.uuid)
pathvers = keys(load_versions(path))
pathvers = keys(load_versions(path; include_yanked = true)) # ??
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Probably false here.

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, what does this do? So we can add a test

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It is for backward compatibility with 0.6 stuff so not sure we need to add a test really. It just sets the version to the maximum one in the registry, only used when no Project file is present.

@StefanKarpinski
Copy link
Sponsor Member

I wonder whether we should have this include_yanked option at all? Do we need it for anything?

@fredrikekre
Copy link
Member Author

Do we need it for anything?

We need to collect yanked versions when instantiating a Manifest (I think).

@fredrikekre
Copy link
Member Author

... but maybe we can go look directly for the tree hash instead of first collecting all versions

@StefanKarpinski
Copy link
Sponsor Member

If it complicates things not having it, keeping is fine, I was just wondering about it.

@fredrikekre fredrikekre added registries Pkg's handling of package registries and removed triage labels Dec 2, 2018
@fredrikekre
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Dec 2, 2018
@bors
Copy link
Contributor

bors bot commented Dec 2, 2018

@fredrikekre
Copy link
Member Author

If it complicates things not having it, keeping is fine, I was just wondering about it.

Let's leave it there for now, can be revisited.

bors r+

bors bot added a commit that referenced this pull request Dec 4, 2018
726: Implement option to yank/blacklist a release. r=fredrikekre a=fredrikekre

Implement support for yanking versions in a package registry.



938: better error message when we cant find uuid in manifest r=fredrikekre a=KristofferC

This should not really happen but it seems to happen anyway, so at least give a better error message for it.

Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
@bors
Copy link
Contributor

bors bot commented Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
registries Pkg's handling of package registries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a mechanism to mark a release as broken
5 participants