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

Fix --build-root option incorrectly installing to user dir inside build root #7212

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Nov 30, 2023

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

I was hoping that this changes would fix #7083, but I can't reproduce the issue either in current master.

Changes in #5327 made --build-root start installing into user dir inside the build root.

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

It builds the fix on a change in the approach to #5327 that makes it easier to fix this kind of issue and should also be more robust, since it does not require to keep a list of commands that the warning applies to, and to save whether we fallback or not to check the result somewhere else.

With the new approach in place, it gets simpler to fix the problem, since build-root is prepended in the exact same place where the fallback to installing to the user directory happens.

Fixes #7083.

Make sure the following tasks are checked

@deivid-rodriguez deivid-rodriguez changed the title Better approach to implement falling back to user install when gem home is not writable Fix --build-root option incorrectly installing to user dir inside build root Nov 30, 2023
@deivid-rodriguez deivid-rodriguez marked this pull request as draft November 30, 2023 23:11
@duckinator
Copy link
Member

@deivid-rodriguez here's a patch that takes this PR and both fixes --build-root and adds a test for it: 0001-Make-build-root-disable-auto-user-install.patch

If you'd like, I can push the changes myself.

Otherwise, you can download the patch file and run git am 0001-Make-build-root-disable-auto-user-install.patch.

(Since this is your PR, I didn't want to push changes without your permission.)

@deivid-rodriguez
Copy link
Member Author

Your patch looks good but broke a couple of tests. I pushed a small tweak to fix that. Also noticing that the test you added already passes without the fix? Looking into that now too.

@deivid-rodriguez
Copy link
Member Author

I amended the test so that it properly reproduces the problem, and also added another test to the change in approach to the fallback. I had removed all previous tests because they were dependent on the previous implementation but I had not added a new test to cover the new approach. That's now in place too!

@deivid-rodriguez
Copy link
Member Author

@duckinator This is ready from my side. I'd just like to give this a final rebase & some commit squashing, but I'll wait for a final review pass from you in case you have some comments!

@duckinator
Copy link
Member

@deivid-rodriguez rebase and squash away. 👍

@deivid-rodriguez deivid-rodriguez merged commit 298def5 into master Dec 5, 2023
60 checks passed
@deivid-rodriguez deivid-rodriguez deleted the fix-build-root branch December 5, 2023 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--build-root option is not respected anymore
3 participants