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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

formulary: migrate fully-scoped formulae names. #1371

Merged
merged 1 commit into from
Nov 6, 2016
Merged

formulary: migrate fully-scoped formulae names. #1371

merged 1 commit into from
Nov 6, 2016

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Oct 24, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

e.g. allow brew install some/tap/formula to look that formula up in
another tap if it has been migrated.

CC @ilovezfs @tdsmith this will mean we can migrate formulae from taps and have e.g. brew install homebrew/python/pillow not go 馃挜

@MikeMcQuaid MikeMcQuaid added the in progress Maintainers are working on this label Oct 24, 2016
@ilovezfs
Copy link
Contributor

Is this going to mean keeping dead/empty (or mostly dead/empty) taps around "forever" as a registry, and then causing it to break if/when the repo is ever removed?

If so, let's just put the registry/registries in brew itself in a structured file/files, not rely on zombie repositories to serve as alias databases.

@zmwangx
Copy link
Contributor

zmwangx commented Oct 25, 2016

let's just put the registry/registries in brew itself in a structured file/files

Don't forget tap migrations is a feature that's available to third party taps as well.

I would suggest keeping the info in the destination tap as well, so that if the source tap is unavailable, we can still track down the destination if the destination tap is tapped. This at the very least solves the third-party-tap-incorporated-into-core problem.

@MikeMcQuaid
Copy link
Member Author

Is this going to mean keeping dead/empty (or mostly dead/empty) taps around "forever" as a registry, and then causing it to break if/when the repo is ever removed?

In the short-term: yes. In the long-term: no.

Don't forget tap migrations is a feature that's available to third party taps as well.

Indeed.

@ilovezfs
Copy link
Contributor

I would suggest keeping the info in the destination tap as well

I agree with this. Relying on the original taps, even in the short-run, seems like a mistake when the needed information is available and can be kept in brew or core.

@MikeMcQuaid
Copy link
Member Author

Relying on the original taps, even in the short-run, seems like a mistake when the needed information is available and can be kept in brew or core.

We will need both approaches for the cases where a migration happens between a tap that's tapped and one that is not.

@ilovezfs
Copy link
Contributor

If pillow moves to homebrew/core,

depends_on "homebrew/python/pillow"

should not require tapping homebrew/python to figure out that it should use the homebrew/core pillow.

@MikeMcQuaid
Copy link
Member Author

If some random, non Homebrew-org tap wants to move a formula from one tap to another it shouldn't require us to make a change in homebrew-core or Homebrew/brew so this change will still be needed regardless. Even when moving things from taps to homebrew/core I don't know how I feel about maintaining migration lists in multiple places.

Something that's not here yet but will be added to this PR is a user-facing deprecation warning to fix up the dependency/Brewfile/install line accordingly.

@ilovezfs
Copy link
Contributor

If some random, non Homebrew-org tap wants to move a formula from one tap to another it shouldn't require us to make a change in homebrew-core or Homebrew/brew so this change will still be needed regardless.

That's not the use case we currently face, and supporting insecure redirects from third parties to fourth parties is not exactly a high priority.

e.g. allow `brew install some/tap/formula` to look that formula up in
another tap if it has been migrated.

Also, add a deprecation message to point people towards the correct
naming.
@MikeMcQuaid MikeMcQuaid merged commit ce2b11f into Homebrew:master Nov 6, 2016
@MikeMcQuaid MikeMcQuaid deleted the tap-migrate-fully-scoped branch November 6, 2016 13:22
@MikeMcQuaid MikeMcQuaid removed the in progress Maintainers are working on this label Nov 6, 2016
@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 6, 2016

Um, ok ...

@MikeMcQuaid
Copy link
Member Author

@ilovezfs This is an improvement as-is and we're going to need this even if we additionally decide to have these migrations live in e.g. homebrew/core.

@MikeMcQuaid
Copy link
Member Author

@ilovezfs If/when we do kill homebrew/versions I'll take the approach you've mentioned where this logic will live in homebrew/core.

@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 6, 2016

I just don't like the precedent of "silent" merges when the conversation was left as it was above.

@MikeMcQuaid
Copy link
Member Author

Apologies, I thought the conversation had reached a conclusion in that we'd disagreed on the short-term approach but agreed on the long-term one. This is ready to go and working so I don't think it makes sense to block this (already pretty delayed) work on the long-term version.

@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 6, 2016

Yup, agreed. Just like things to end with 馃樃 not 馃槨

No apologies needed.

@ilovezfs
Copy link
Contributor

It looks like this isn't able to handle the homebrew/versions/glfw3 -> homebrew/core/glfw migration: https://bot.brew.sh/job/Homebrew%20Testing/1219/version=el_capitan/console

@zmwangx
Copy link
Contributor

zmwangx commented Dec 30, 2016

Migration and rename shouldn't be mutually exclusive.

@zmwangx
Copy link
Contributor

zmwangx commented Dec 30, 2016

Yeah, the migration & rename entries are correct, but migration and rename are handled as mutually exclusive cases in the code: https://github.com/Homebrew/brew/blob/c3a2bf3/Library/Homebrew/formulary.rb#L182-L197.

@ilovezfs
Copy link
Contributor

@zmwangx I think the bug may be specific to test-bot though

iMac-TMP:~ joe$ brew install -s arrayfire
==> Installing arrayfire from homebrew/science
==> Tapping homebrew/versions
Cloning into '/usr/local/Homebrew/Library/Taps/homebrew/homebrew-versions'...
remote: Counting objects: 210, done.
remote: Compressing objects: 100% (207/207), done.
remote: Total 210 (delta 17), reused 33 (delta 2), pack-reused 0
Receiving objects: 100% (210/210), 246.21 KiB | 264.00 KiB/s, done.
Resolving deltas: 100% (17/17), done.
Tapped 201 formulae (231 files, 898.6K)
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
==> Installing dependencies for homebrew/science/arrayfire: boost-compute, clblas, clfft, homebrew/versions/glfw3
==> Installing homebrew/science/arrayfire dependency: boost-compute
==> Downloading https://homebrew.bintray.com/bottles-science/boost-compute-1.62.0.el_capitan.bottle.tar.gz
######################################################################## 100.0%
==> Pouring boost-compute-1.62.0.el_capitan.bottle.tar.gz
馃嵑  /usr/local/Cellar/boost-compute/1.62.0: 318 files, 1.3M
==> Installing homebrew/science/arrayfire dependency: clblas
==> Downloading https://homebrew.bintray.com/bottles-science/clblas-2.6.el_capitan.bottle.tar.gz
######################################################################## 100.0%
==> Pouring clblas-2.6.el_capitan.bottle.tar.gz
馃嵑  /usr/local/Cellar/clblas/2.6: 20 files, 4M
==> Installing homebrew/science/arrayfire dependency: clfft
==> Downloading https://homebrew.bintray.com/bottles-science/clfft-2.12.1.el_capitan.bottle.tar.gz
######################################################################## 100.0%
==> Pouring clfft-2.12.1.el_capitan.bottle.tar.gz
馃嵑  /usr/local/Cellar/clfft/2.12.1: 23 files, 1.1M
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
==> Installing homebrew/science/arrayfire dependency: homebrew/versions/glfw3
==> Downloading https://homebrew.bintray.com/bottles/glfw-3.2.1.el_capitan.bottle.tar.gz
######################################################################## 100.0%
==> Pouring glfw-3.2.1.el_capitan.bottle.tar.gz
馃嵑  /usr/local/Cellar/glfw/3.2.1: 14 files, 286.5K
==> Installing homebrew/science/arrayfire 
==> Using the sandbox
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
Warning: Use glfw instead of deprecated homebrew/versions/glfw3
==> Cloning https://github.com/arrayfire/arrayfire.git
Updating /Users/joe/Library/Caches/Homebrew/arrayfire--git
==> Checking out tag v3.3.2
==> Downloading https://downloads.sourceforge.net/project/glew/glew/1.13.0/glew-1.13.0.tgz
Already downloaded: /Users/joe/Library/Caches/Homebrew/arrayfire--glew1-1.13.0.tgz
==> make GLEW_PREFIX=/usr/local/Cellar/arrayfire/3.3.2/libexec/glew1 GLEW_DEST=/usr/local/Cellar/arrayfire/3.3.2/libexec/glew1 all

@zmwangx
Copy link
Contributor

zmwangx commented Dec 30, 2016

Hmm, okay, isn't immediately obvious from the code... Good thing it's not user facing 馃

@ilovezfs
Copy link
Contributor

@zmwangx I think it's the new aggressive untapping behavior which is causing test-bot to shoot itself in the foot:

==> brew uses --recursive homebrew/science/arrayfire
==> Tapping homebrew/versions
Cloning into '/usr/local/Homebrew/Library/Taps/homebrew/homebrew-versions'...
Tapped 201 formulae (230 files, 3M)
==> git checkout master -f
==> brew cleanup --prune=7
==> git clean -ffdx --exclude=Library/Taps
Untapping homebrew/versions... (230 files, 3M)
Untapped 201 formulae
Already on 'master'
HEAD is now at 4ca2eaf Merge pull request #1682 from MikeMcQuaid/tap_migrations_rename
Already on 'master'
Your branch is up-to-date with 'origin/master'.
HEAD is now at 9d524e4 thrift: update 0.9.3 bottle.
Already on 'master'
Your branch is up-to-date with 'origin/master'.
HEAD is now at 121855e Merge pull request #46 from MikeMcQuaid/cleanup-top-level-dirs
Error: No available formula with the name "homebrew/versions/glfw3" 
Please tap it and then try again: brew tap homebrew/versions
Build step 'Execute shell' marked build as failure

@MikeMcQuaid
Copy link
Member Author

Yeah, the migration & rename entries are correct, but migration and rename are handled as mutually exclusive cases in the code:

That's not correct, the migration case calls back into formula_name_path specifically to handle migrations and renames.

MikeMcQuaid pushed a commit to zmwangx/brew that referenced this pull request Mar 20, 2017
Partial implementation of
https://github.com/Homebrew/brew-evolution/pull/15, along with the ability to
search for deleted formulae in git history (inspired by Homebrew#1996) which is not
described in the proposal.

See also: Homebrew#1371.
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants