Skip to content

Fix some invalid options to gem DSL not getting reported as invalid #8480

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 12, 2025
Merged

Fix some invalid options to gem DSL not getting reported as invalid #8480

merged 2 commits into from
Feb 12, 2025
+111 −86

Conversation

@deivid-rodriguez
Copy link
Contributor

What was the end-user or developer problem that led to this PR?

I found this while working on something else. type and gemfile are internally handled as valid options for the gem DSL, but it was only due to an implementation detail, they are not should not be exposed to end users.

What is your fix for the problem, implemented in this PR?

My fix is to refactor the way we handle the gemspec DSL to skip option validation (since we're passing known options. That way, we can stop considering gemfile and type as valid and give better errors if they are ever used for whatever reason.

Make sure the following tasks are checked

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/fix-type-and-gemfile-option-validation branch from d16e1af to aaa40f5 Compare February 10, 2025 19:04
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/fix-type-and-gemfile-option-validation branch from aaa40f5 to 5b6077a Compare February 10, 2025 19:46
@deivid-rodriguez deivid-rodriguez merged commit 95f58b9 into master Feb 12, 2025
91 checks passed
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/fix-type-and-gemfile-option-validation branch February 12, 2025 11:17
deivid-rodriguez added a commit that referenced this pull request Feb 17, 2025
@deivid-rodriguez
Merge pull request #8480 from rubygems/deivid-rodriguez/fix-type-and-…
048b055 
…gemfile-option-validation

Fix some invalid options to `gem` DSL not getting reported as invalid

(cherry picked from commit 95f58b9)
ccutrer added a commit to ccutrer/rubygems that referenced this pull request Apr 9, 2025
@ccutrer
Fix plugin dep handling
8c0fccf 
Since ruby#8480, we can't use the undocumented "type" option, but the
add_dependency helper was added that lets us easily validate first,
then add our custom options, then directly add the now custom
dependency.

This conveniently simplifies the Plugin version of the DSL, because
`plugin` no longer flows through `gem`, so it doesn't need to special
case it.
myronmarston pushed a commit to block/elasticgraph that referenced this pull request Jul 8, 2025
chore: upgrade bundler to 2.6.9.
b5c8be1 
The new version of bundler includes some internal refactorings that
broke our monorepo setup. To stay compatible, I've had to rework our
`gem` override to instead override `add_dependency`.

I believe this is the root of the problem I reported to dependabot:

dependabot/dependabot-core#12426

The latest dependabot uses Bundler 2.6.9, and this was causing the
issues reported in that ticket since our `gem` override was no longer
effective.

The specific bundler change prompting this is this PR:

ruby/rubygems#8480

In that PR, the `gemspec` method was refactored to no longer call `gem`.
Instead, it calls a new `add_dependency` method.
@myronmarston myronmarston mentioned this pull request Jul 8, 2025
Closed
myronmarston pushed a commit to block/elasticgraph that referenced this pull request Jul 8, 2025
chore: rework Gemfiles.
aa11cd3 
Dependabot is currently broken for ElasticGraph:

dependabot/dependabot-core#12426

Troubleshooting points to a root cause of Bundler 2.6.9 (used by
the latest dependabot) having some subtle changes to how gemspecs
are handled. Specifically, this PR:

ruby/rubygems#8480

In that PR, the `gemspec` method was refactored to no longer call `gem`.

Previously, our `Gemfile` set up depended on this. However, it was quite
complicated in order to support the root `Gemfile` getting symlinkd into
each gem subdirectory.

I've simplified things here:

- `Gemfile-shared` now contains all dependencies that are not declared in the gemspecs.
- `Gemfile` is now greatly simplified--it just evals `Gemfile-shared` and delcares
  a dependency on each ElasticGraph gem.
- `config/Gemfile-for-gem` contains a more complex gemfile, which gets symlinked
  into each ElasticGraph gem.

This new simpler setup should fix the issues we're having with dependabot.
@myronmarston myronmarston mentioned this pull request Jul 8, 2025
Merged
myronmarston pushed a commit to block/elasticgraph that referenced this pull request Jul 8, 2025
chore: rework Gemfiles.
c22b861 
Dependabot is currently broken for ElasticGraph:

dependabot/dependabot-core#12426

Troubleshooting points to a root cause of Bundler 2.6.9 (used by
the latest dependabot) having some subtle changes to how gemspecs
are handled. Specifically, this PR:

ruby/rubygems#8480

In that PR, the `gemspec` method was refactored to no longer call `gem`.

Previously, our `Gemfile` set up depended on this. However, it was quite
complicated in order to support the root `Gemfile` getting symlinkd into
each gem subdirectory.

I've simplified things here:

- `Gemfile-shared` now contains all dependencies that are not declared in the gemspecs.
- `Gemfile` is now greatly simplified--it just evals `Gemfile-shared` and delcares
  a dependency on each ElasticGraph gem.
- `config/Gemfile-for-gem` contains a more complex gemfile, which gets symlinked
  into each ElasticGraph gem.
- I've removed the last few gemspec developemnt dependencies for non-ElasticGraph
  gems--we handle this through the development dependencies in `Gemfile-shared` instead.
  (`rack-test` was missing from the `Gemfile` before!)

This new simpler setup should fix the issues we're having with dependabot.
myronmarston pushed a commit to block/elasticgraph that referenced this pull request Jul 8, 2025
chore: rework Gemfiles.
4bc24f8 
Dependabot is currently broken for ElasticGraph:

dependabot/dependabot-core#12426

Troubleshooting points to a root cause of Bundler 2.6.9 (used by
the latest dependabot) having some subtle changes to how gemspecs
are handled. Specifically, this PR:

ruby/rubygems#8480

In that PR, the `gemspec` method was refactored to no longer call `gem`.

Previously, our `Gemfile` set up depended on this. However, it was quite
complicated in order to support the root `Gemfile` getting symlinkd into
each gem subdirectory.

I've simplified things here:

- `Gemfile-shared` now contains all dependencies that are not declared in the gemspecs.
- `Gemfile` is now greatly simplified--it just evals `Gemfile-shared` and declares
  a dependency on each ElasticGraph gem.
- `config/Gemfile-for-gem` contains a more complex gemfile, which gets symlinked
  into each ElasticGraph gem.
- I've removed the last few gemspec developemnt dependencies for non-ElasticGraph
  gems--we handle this through the development dependencies in `Gemfile-shared` instead.
  (`rack-test` was missing from the `Gemfile` before!)

This new simpler setup should fix the issues we're having with dependabot.
myronmarston pushed a commit to block/elasticgraph that referenced this pull request Jul 8, 2025
chore: rework Gemfiles.
882f28f 
Dependabot is currently broken for ElasticGraph:

dependabot/dependabot-core#12426

Troubleshooting points to a root cause of Bundler 2.6.9 (used by
the latest dependabot) having some subtle changes to how gemspecs
are handled. Specifically, this PR:

ruby/rubygems#8480

In that PR, the `gemspec` method was refactored to no longer call `gem`.

Previously, our `Gemfile` set up depended on this. However, it was quite
complicated in order to support the root `Gemfile` getting symlinkd into
each gem subdirectory.

I've simplified things here:

- `Gemfile-shared` now contains all dependencies that are not declared in the gemspecs.
- `Gemfile` is now greatly simplified--it just evals `Gemfile-shared` and declares
  a dependency on each ElasticGraph gem.
- `config/Gemfile-for-gem` contains a more complex gemfile, which gets symlinked
  into each ElasticGraph gem.
- I've removed the last few gemspec developemnt dependencies for non-ElasticGraph
  gems--we handle this through the development dependencies in `Gemfile-shared` instead.
  (`rack-test` was missing from the `Gemfile` before!)

This new simpler setup should fix the issues we're having with dependabot.
myronmarston pushed a commit to block/elasticgraph that referenced this pull request Jul 8, 2025
chore: rework Gemfiles.
91748a9 
Dependabot is currently broken for ElasticGraph:

dependabot/dependabot-core#12426

Troubleshooting points to a root cause of Bundler 2.6.9 (used by
the latest dependabot) having some subtle changes to how gemspecs
are handled. Specifically, this PR:

ruby/rubygems#8480

In that PR, the `gemspec` method was refactored to no longer call `gem`.

Previously, our `Gemfile` set up depended on this. However, it was quite
complicated in order to support the root `Gemfile` getting symlinkd into
each gem subdirectory.

I've simplified things here:

- `Gemfile-shared` now contains all dependencies that are not declared in the gemspecs.
- `Gemfile` is now greatly simplified--it just evals `Gemfile-shared` and declares
  a dependency on each ElasticGraph gem.
- `config/Gemfile-for-gem` contains a more complex gemfile, which gets symlinked
  into each ElasticGraph gem.
- I've removed the last few unnecesary gemspec developemnt dependencies for non-ElasticGraph
  gems--we handle this through the development dependencies in `Gemfile-shared` instead.
  (`rack-test` was missing from the `Gemfile` before!)

This new simpler setup should fix the issues we're having with dependabot.
myronmarston pushed a commit to block/elasticgraph that referenced this pull request Jul 8, 2025
chore: rework Gemfiles. (#645)
cce2474 
Dependabot is currently broken for ElasticGraph:

dependabot/dependabot-core#12426

Troubleshooting points to a root cause of Bundler 2.6.9 (used by
the latest dependabot) having some subtle changes to how gemspecs
are handled. Specifically, this PR:

ruby/rubygems#8480

In that PR, the `gemspec` method was refactored to no longer call `gem`.

Previously, our `Gemfile` set up depended on this. However, it was quite
complicated in order to support the root `Gemfile` getting symlinkd into
each gem subdirectory.

I've simplified things here:

- `Gemfile-shared` now contains all dependencies that are not declared in the gemspecs.
- `Gemfile` is now greatly simplified--it just evals `Gemfile-shared` and declares
  a dependency on each ElasticGraph gem.
- `config/Gemfile-for-gem` contains a more complex gemfile, which gets symlinked
  into each ElasticGraph gem.
- I've removed the last few unnecesary gemspec developemnt dependencies for non-ElasticGraph
  gems--we handle this through the development dependencies in `Gemfile-shared` instead.
  (`rack-test` was missing from the `Gemfile` before!)

This new simpler setup should fix the issues we're having with dependabot.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bundler: enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@deivid-rodriguez