Shopify / tapioca

The swiss army knife of RBI generation
MIT License
746 stars 128 forks source link

Difference in gems per architecture causes --verify to fail on CI #658

Closed bmulholland closed 2 years ago

bmulholland commented 2 years ago

nokogiri has dependencies that are different by architecture:

    nokogiri (1.12.5)
      mini_portile2 (~> 2.6.1)
      racc (~> 1.4)
    nokogiri (1.12.5-x86_64-darwin)
      racc (~> 1.4)
    nokogiri (1.12.5-x86_64-linux)
      racc (~> 1.4)

When I run tapioca gem on my mac, an RBI is generated for mini_portile2. However, when I run gem --verify on CI, that architecture doesn't include mini_portile2, so it's marked as "removed" and the verify fails.

For now I've excluded the gem from the local gem RBI generation, so it's just not there, but that feels wrong. Slightly preferable is to tell verify to ignore the gem altogether, but there's only the exclude option, which is effectively already being used on CI. So what's best here?

KaanOzkan commented 2 years ago

I may be misunderstanding but using --exclude with --verify seems to be a good solution. You can keep the mini_portile2 rbi in the repo and get typechecking benefits without encountering the error from --verify. Does that not work as intended?

bmulholland commented 2 years ago

AFAICT, it works as intended, but not as you described.

If the gem RBI is in the repo, then tapioca --verify passes -- but tapioca --verify --exclude=mini_portile2 fails:

Reason:
  File(s) removed:
  - sorbet/rbi/gems/sorbet/rbi/gems/mini_portile2@2.6.1.rbi
KaanOzkan commented 2 years ago

Thanks for this, had a quick look at the --verify code and you're right. I'd like to support this and I think making --verify logic consider the arguments to --exclude is a good solution.

Currently --exclude is only considered during the RBI generation done by verify, therefore we generate RBIs without mini_portile2 and then we simply compare the RBIs generated with existing RBIs under sorbet/rbi/gems/ which outputs an error.

bmulholland commented 2 years ago

I agree that exclude would be a good fit here; the arg just means something different than it currently does in the verify context