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

Resolve ruby version file relative to bundle root #6892

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

ngan
Copy link
Contributor

@ngan ngan commented Aug 18, 2023

This is a follow up to #6876. This change makes it so that the version file is resolved relative to the Bundle root instead of the working directory.

Why is this useful?

If you run a commnad (eg rails) from the app/ directory, your bundle would fail to load.

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

Running rails from a subdirectory in your project would fail because the version file would be resolved in the working directory...and therefore be not found.

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

Resolve from Bundle.root.

Make sure the following tasks are checked

This is a follow up to rubygems#6742.
This change makes it so that the version file is resolved relative to
the Bundle root instead of the working directory.

Why is this useful?

If you run a commnad (eg `rails`) from the `app/` directory, your bundle
would fail to load.
@ngan
Copy link
Contributor Author

ngan commented Aug 18, 2023

@deivid-rodriguez @martinemde I attempted to use ruby file: "..." but ran into a tiny issue. This fixes it.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the quick fix!


before do
allow(Bundler).to receive(:read_file).with(project_root.join("foo")).and_return("#{version}\n")
allow(Bundler).to receive(:root).and_return(Pathname.new("/path/to/project"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
allow(Bundler).to receive(:root).and_return(Pathname.new("/path/to/project"))
allow(Bundler).to receive(:root).and_return(project_root)

Just a nitpick.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, Martin! Want me to open another PR to fix or you will just fix it on master?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very tiny. Maybe it can get thrown into a follow up PR. #6895 wants to add .tool-versions support.

@martinemde martinemde merged commit cabe7bb into rubygems:master Aug 18, 2023
92 checks passed
@martinemde
Copy link
Member

I'm ignoring my nitpick and getting this merged so that the feature works on master as intended. Thank you for testing and fixing this so quickly!

deivid-rodriguez pushed a commit that referenced this pull request Sep 21, 2023
Resolve ruby version file relative to bundle root

(cherry picked from commit cabe7bb)
deivid-rodriguez pushed a commit that referenced this pull request Sep 21, 2023
Resolve ruby version file relative to bundle root

(cherry picked from commit cabe7bb)
@ngan ngan deleted the improve-ruby-file-path branch February 8, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants