Migration to other tap broken about brew HOT 10 OPEN

I think your tap_migrations.json in pe/old-tap should be

{
    "example": "pe/new-tap/example"
}

Looking at other examples I don't think so: https://github.com/brewsci/homebrew-science/blob/5d14d2fda3934c0eb8243c1cbdb9fb4621ec110b/tap_migrations.json

I tried it with pe/homebrew-old-tap@cc86591. Same error.

from brew.

I think your tap_migrations.json in pe/old-tap should be

{
    "example": "pe/new-tap/example"
}

from brew.

I think I found the source of the issue. The same cask is found twice. Once in the new tap and once in the old but mapped to the new tap.

If I inspect loaders from this code:

I get this:

[
#<Cask::CaskLoader::FromNameLoader:0x0000000125cac788 @token="example", @path=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Casks/example.rb>, @tap=#<Tap:0x0000000122de3898 @user="pe", @repo="new-tap", @name="pe/new-tap", @full_name="pe/homebrew-new-tap", @path=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap>, @git_repo=#<GitRepository:0x0000000122e0f6f0 @pathname=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap>>, @alias_dir=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Aliases>, @alias_files=[], @alias_table={}, @formula_renames={}, @tap_migrations={}, @potential_formula_dirs=[#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Formula>, #<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/HomebrewFormula>, #<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap>], @formula_dir=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap>, @formula_files=[], @formula_files_by_name={}, @cask_renames={}, @cask_dir=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Casks>, @cask_files=[#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Casks/example.rb>], @cask_files_by_name={"example"=>#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Casks/example.rb>}>>,
#<Cask::CaskLoader::FromNameLoader:0x0000000125caa410 @token="example", @path=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Casks/example.rb>, @tap=#<Tap:0x0000000122de3898 @user="pe", @repo="new-tap", @name="pe/new-tap", @full_name="pe/homebrew-new-tap", @path=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap>, @git_repo=#<GitRepository:0x0000000122e0f6f0 @pathname=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap>>, @alias_dir=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Aliases>, @alias_files=[], @alias_table={}, @formula_renames={}, @tap_migrations={}, @potential_formula_dirs=[#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Formula>, #<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/HomebrewFormula>, #<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap>], @formula_dir=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap>, @formula_files=[], @formula_files_by_name={}, @cask_renames={}, @cask_dir=#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Casks>, @cask_files=[#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Casks/example.rb>], @cask_files_by_name={"example"=>#<Pathname:/opt/homebrew/Library/Taps/pe/homebrew-new-tap/Casks/example.rb>}>>
]

The only difference is the object_id.

Changing the above code like so fixes the issue.

loaders = Tap.select { |tap| tap.installed? && !tap.core_cask_tap? }
             .filter_map { |tap| super("#{tap}/#{token}", warn:) }
             .select { |tap| tap.path.exist? }
             .uniq{ |cask| cask.tap } # this line was added by me

But my knowledge of this codebase is none and my ruby is very limited. I can't judge if this is a good idea or not.

How to proceed? Should I just create a PR and we'll discuss there? Would you want any tests for this change? Where? Are there other places where this is also necessary?

from brew.

I can't judge if this is a good idea or not.

My Ruby-fu is limited too, but I'm pretty sure that:

• the whole point of migrations is that the formula/cask no longer exists in the old tap
• taps are processed in alpha-sorted order, so if your old tap sorts before the new one (e.g. pe/tap1 -> pe/tap2), your fix will pick the wrong tap

from brew.

• the whole point of migrations is that the formula/cask no longer exists in the old tap

Yes, this. You should delete the formula/cask from the old file or it won't work as expected.

from brew.

How to proceed? Should I just create a PR and we'll discuss there?

Yes, this would be best.

Would you want any tests for this change? Where?

Ideally the corresponding _spec.rb file to the source file(s) you edit. That can wait until we've validated the approach with a draft PR, though.

from brew.

taps are processed in alpha-sorted order, so if your old tap sorts before the new one (e.g. pe/tap1 -> pe/tap2), your fix will pick the wrong tap

I don't think that the order matters because both casks are identical (except for the object_id). The cask from the old tap was already migrated to the new one, that's why they are identical.

• the whole point of migrations is that the formula/cask no longer exists in the old tap

Yes, this. You should delete the formula/cask from the old file or it won't work as expected.

I did delete it in the old tap and get this error nevertheless. See What were you trying to do (and why)?.

How to proceed? Should I just create a PR and we'll discuss there?

Yes, this would be best.

OK. Will do.

from brew.

I suspect what's happening here is that we're getting identical cask loaders because the call to super evaluates cask renames and migrations so it's possible that the original tap that the cask was migrated from is returning the cask loader for the new tap.

The super call leads to the Cask::CaskLoader::FromTapLoader.try_new method which calls the Cask::CaskLoader.tap_cask_token_type method. Normally it just returns the cask token, tap and type specified but if the cask has been renamed or migrated it returns the new token, tap and type. Since they are identical, just removing duplicates by path makes sense here. Tap would also work but path is a bit more specific.

Ideally, we'd figure out how to refactor this so that this doesn't happen in this sort of situation but I'm not really sure what that would look like.

from brew.

While playing around, I just noticed that the same error also applies to formulae, not only to casks.

Step-by-step reproduction instructions (by running brew commands)

https://github.com/pe/homebrew-old-tap/blob/5a0e91e527c5962096e62fa277c896a1c285faef/tap_migrations.json#L3
https://github.com/pe/homebrew-new-tap/blob/d755222b381ccbea385f7884b97b9b8b9f19429e/Formula/examplef.rb#L1

~ ❯ brew install examplef --verbose --debug
Warning: Formula pe/old-tap/examplef was renamed to pe/new-tap/examplef.
Error: Formulae found in multiple taps:
       * pe/new-tap/examplef
       * pe/new-tap/examplef

Please use the fully-qualified name (e.g. pe/new-tap/examplef) to refer to a specific formula.
/opt/homebrew/Library/Homebrew/formulary.rb:837:in `try_new'
/opt/homebrew/Library/Homebrew/formulary.rb:1227:in `block in loader_for'
/opt/homebrew/Library/Homebrew/formulary.rb:1226:in `each'
/opt/homebrew/Library/Homebrew/formulary.rb:1226:in `loader_for'
/opt/homebrew/Library/Homebrew/formulary.rb:1008:in `factory'
/opt/homebrew/Library/Homebrew/cli/parser.rb:709:in `block in formulae'
/opt/homebrew/Library/Homebrew/cli/parser.rb:705:in `each'
/opt/homebrew/Library/Homebrew/cli/parser.rb:705:in `filter_map'
/opt/homebrew/Library/Homebrew/cli/parser.rb:705:in `formulae'
/opt/homebrew/Library/Homebrew/cli/parser.rb:363:in `parse'
/opt/homebrew/Library/Homebrew/abstract_command.rb:53:in `initialize'
/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.3.0/gems/sorbet-runtime-0.5.11385/lib/types/private/abstract/declare.rb:37:in `new'
/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.3.0/gems/sorbet-runtime-0.5.11385/lib/types/private/abstract/declare.rb:37:in `block in declare_abstract'
/opt/homebrew/Library/Homebrew/brew.rb:90:in `<main>'

The exact same code also exists here:

from brew.