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

automatic merging guidelines #3673

Closed
StefanKarpinski opened this issue Jul 16, 2019 · 20 comments
Closed

automatic merging guidelines #3673

StefanKarpinski opened this issue Jul 16, 2019 · 20 comments

Comments

@StefanKarpinski
Copy link
Contributor

These guidelines are intended not as requirements for packages but as very conservative guidelines, which, if your new package or new version of a package meets them, it may be automatically merged.

New package

  1. Normal capitalization
    • name should match r"^[A-Z]\w*[a-z][0-9]?$"
      • i.e. starts with a capital letter, ASCII alphanumerics only, ends in lowercase
  2. Not too short
    • at least five letters
      • you can register names shorter than this, but doing so requires someone to approve
  3. Standard initial version number
    • one of 0.0.1, 0.1.0, 1.0.0
  4. Repo URL ends with /$name.jl.git where name is the package name

New version

  1. Sequential version number
    • if the last version was 1.2.3 then the next can be 1.2.4, 1.3.0 or 2.0.0
  2. Compat for all dependencies
    • all [deps] should also have [compat] entries (and Julia itself)
    • [compat] entries should have upper bounds
  3. Version can be installed
    • given the proposed changes to the registry, can we resolve and install the new version of the package?
  4. Version can be loaded
    • once it's been installed (and built?), can we load the code?
@ararslan
Copy link
Member

ends in lowercase

? Many packages do not follow this, e.g. HTTP.

@StefanKarpinski
Copy link
Contributor Author

StefanKarpinski commented Jul 16, 2019

These guidelines are intended not as requirements for packages but as very conservative guidelines, which, if your new package or new version of a package meets them, it may be automatically merged.

A package with a name like HTTP would require review.

@ararslan
Copy link
Member

Is that to catch non-obvious acronyms or something?

@StefanKarpinski
Copy link
Contributor Author

Yes, or people who just capitalize an entire package name for no reason, e.g. MYPACAKGE.jl.

@DilumAluthge
Copy link
Member

This may go without saying but: We should also check every pull request to make sure that it only modifies files related to the package it claims to be related to.

For example, a pull request to register a new package Foo.jl would be allowed to create/modify files in the F/Foo directory, and would be allowed to add a line to Registry.toml containing the name, UUID, and path of Foo.jl, but would not be allowed to e.g. create/modify files in the B/Bar directory or modify lines in Registry.toml relating to the Bar.jl or Baz.jl packages. A pull request to register a new version for an existing package Foo.jl would be allowed to create/modify files in the F/Foo directory, but would not be allowed to modify Registry.toml or create/modify any other files.

Otherwise, we would have a security vulnerability:

  • Suppose I maintain an existing package Foo.jl. The current version is v1.0.0.
  • I submit a pull request to register a new version v2.0.0 of my package Foo.jl. So the "sequential version number" requirement is met ✅.
  • I make sure that I have compat entries for all dependencies including Julia, and each compat entry has an upper bound ✅.
  • Version v2.0.0 of Foo.jl is installable ✅ and loadable ✅ .
  • Oh but also, by the way, my pull request changes the URL of OrderedCollections.jl to point to my fork of OrderedCollections.jl into which I have inserted some malicious code.

@StefanKarpinski
Copy link
Contributor Author

We would only automerge PRs made by Registrator, which we control, not PRs made by people—those must be manually reviewed.

@DilumAluthge
Copy link
Member

We would only automerge PRs made by Registrator, which we control, not PRs made by people—those must be manually reviewed.

Perfect!

@DilumAluthge
Copy link
Member

I have opened a pull request that implements these automatic merging guidelines: #3342

@oxinabox
Copy link
Contributor

oxinabox commented Sep 19, 2019

Compat for all dependencies

  • all [deps] should also have [compat] entries (and Julia itself)
  • [compat] entries should have upper bounds

I would like to suggest ammending this to:

Compat for all changed/added dependencies

  • all new [deps] should also have [compat] entries (and Julia itself)
  • modified [compat] entries should have upper bounds

Basically, if you didn't touch a dep or its compat, then you should still get auto-merge.
I am a big fan of caret bounding everything,
but exceptions can apply.

For example lets say my package depends on DataFrames.jl,
but the only thing I actually depend upon is that DataFrames exports the AbstractDataFrame type, because I have some type-constraint related to that.
E.g. I just define a trait obsdim(::Type{AbstractDataFrame})=2).

So I only lower-bound it, because I am pretty confidant that DataFrames is not going to stop exporting that type.
Then when I tag a release for the first time, it is blocked from automerge because of this,
and I explain to the General registry maintainer why I am only lower-bounding it.
And they are like "Yeah, makes sense. [merge]".

Now later I update my package, I haven't changed how I use DataFrames,
I don't want to be blocked and having to reexplain each time why.

But If later I do bump my minimum DataFrames compat, then I should be stopped since my past evidence has been proven wrong, and I do need the upper-bounds.
and also because I need to go an ret-con the registry to say that all my old version also should be upper bounded.

@fredrikekre fredrikekre transferred this issue from JuliaRegistries/Registrator.jl Sep 19, 2019
@DilumAluthge
Copy link
Member

Personally, I would like to keep the automerging guidelines as conservative as possible.

The goal (at least as I envision it) is not to automerge every package, but rather to automerge many packages.

The use case that you describe is totally fine, but I think it’s okay to have those PR’s be manually merged.

@DilumAluthge
Copy link
Member

@oxinabox The other thing we can do in your specific use case is set the compat for DataFrames to something like 0.18.0 <= x < 1.0.0

@oxinabox
Copy link
Contributor

The use case that you describe is totally fine, but I think it’s okay to have those PR’s be manually merged.

Manually merged the first time.
But after the 15th?
Not so much.

It is fine for packages on slow release cycles,
but when practicing continous delivery of packages with dependencies being concurently devleoped,
it isn't unreasonable to release every day for a week.

@oxinabox The other thing we can do in your specific use case is set the compat for DataFrames to something like 0.18.0 <= x < 1.0.0

That is not a legal compat specifier, AFAIK.

Could say: <=100.0 but that means not allowe a lower bound.
Alos: I think we should ideally not automerge things like that too, since it is probably a typo.

@DilumAluthge
Copy link
Member

That is not a legal compat specifier, AFAIK.

It will be at some point. See JuliaLang/Pkg.jl#1232

@staticfloat
Copy link
Member

Things I need for JLL package merging to be automatic:

  • We need _ to be allowed in package names (because all JLL packages will end in _jll.jl)

  • Non-standard version numbers; because the JLL package versions are linked to the upstream library version, they'll in general not be a nice number like 1.0.0, nor be nice, sequential versions.

My PRs currently come in under @staticfloat, but in the future I'll probably change it to be JuliaCIBot or something like that. Perhaps we can special-case PRs that end in _jll.jl so that they don't have the same version requirements?

@DilumAluthge
Copy link
Member

We can hardcode some exceptions for JLL packages.

That being said, I would prefer that we first figure out the automerging for regular packages, get that merged, and make sure it works in production. Once automerging for regular packages has been deployed in production for a while, and we have fixed all of the bugs, then we can open a separate issue for automerging JLL packages? Does that sound like an okay plan?

@DilumAluthge
Copy link
Member

DilumAluthge commented Oct 2, 2019

Also, to make life a little easier, would it be possible for you to use the same title format as Registrator for your pull request?

For example, Registrator pull request titles look like this:

  • New package: Foo v1.2.3
  • New version: Foo v1.2.3

Could you change the format of your titles to look like this:

  • New package: Ncurses_jll v6.1.0+0
  • New version: Ncurses_jll v6.1.0+0

Do you think that would be possible? It would make my life much easier when I start working on automerging for JLL packages?

@staticfloat
Copy link
Member

Yes; is it strictly necessary to disambiguate New package vs. New version? Especially since the version requirements for a new JLL package will not be stricter, as they are for normal packages?

@DilumAluthge
Copy link
Member

Presumably we will keep the existing requirement that new packages have a 3 day waiting period.

@DilumAluthge
Copy link
Member

@staticfloat I opened an issue specifically dedicated to the automerging of JLL packages: JuliaRegistries/RegistryCI.jl#6

@DilumAluthge
Copy link
Member

@StefanKarpinski @fredrikekre What do you think about closing this issue?

Further issues regarding automerge can go here: https://github.com/JuliaRegistries/GeneralRegistryCI.jl/issues

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 a pull request may close this issue.

5 participants