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

Less exceptional user install #7100

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

voxik
Copy link
Contributor

@voxik voxik commented Oct 24, 2023

This is an attempt to make the user install less exceptional. It does several things:

  1. It removes the arbitrary limitation that the --install-dir and --user-install can't be specified at once on command line. This was not the case internally anyway. Now the --install-dir simple overrides the --user-install.
  2. It refactors the code in an attempt to make it more readable. Therefore now if --build-root option is specified, it is applied to every possible path, including the user install case. I am not 100% convinced that this really is the right thing, OTOH it should not harm anything, because using --build-root is very specific and use it together with --user-install would be exceptional

This is an attempt to address some concerns I have risen in #7083

The main purpose is to put handling of user installation into the same
place as e.g. handling the --build-root option handling. There is no
reason why the --build-root option should not prefix also paths used for
user installation.

Please note that the `util_installer` in
`test_generate_plugins_with_user_install` enforced the `:install_dir`,
which is against what user install is about.
It is not nice to require install directory to be always specified,
while this option is later ignored for user installed gems.

Actually, the next step will be to remove `check_install_dir` check and
let the install dir override the user install.
The combination of `install-dir` and `--user-install` used to be
disabled for no good reason. This even makes problem on Linux
distributions such as Fedora, where `--user-install` is set by default
via operating_system.rb.

The `--install-dir` is already prefered over the `--user-install` by
the implementation, therefore just drop the check.
@voxik voxik force-pushed the less-exceptional-user-install branch from b581da1 to 313b1c5 Compare October 24, 2023 08:52
@voxik
Copy link
Contributor Author

voxik commented Oct 24, 2023

The segfault during Bundler test case run is very likely unrelated to this PR

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.

This makes sense to me, I think!

@duckinator
Copy link
Member

In principle this makes sense to me and I'm in favor of it. Looking at the code, I don't see any obvious issues.

I'll poke at it locally later and see if everything seems good.

@duckinator
Copy link
Member

@deivid-rodriguez if you think this is good to go, feel free to merge. I'm just looking at it from my phone atm, so couldn't try running it myself, is all.

@voxik
Copy link
Contributor Author

voxik commented Oct 25, 2023

Funnily enough, I have discovered --vendor option today (after a decade 🙈) and it also clashes with --user-install in non transparent way:

$ gem install gem2rpm --user-install --vendor
ERROR:  Use --install-dir or --user-install but not both

This PR should also address this issue.

@deivid-rodriguez
Copy link
Member

@duckinator I just looked at this superficially, I'd appreciate if you have a closer look 👍.

@duckinator
Copy link
Member

duckinator commented Oct 25, 2023

I can find no difference in behavior between 3.4.21 (latest release) and 313b1c5 (latest commit on this branch), excluding the intentional one (--install-dir overriding --user-install), so I think we're good to go. 👍

@duckinator
Copy link
Member

Thanks for the PR, @voxik. This is quite a nice improvement. ^.^

@duckinator duckinator merged commit 2314d28 into rubygems:master Oct 25, 2023
83 checks passed
@duckinator
Copy link
Member

@voxik turns out you also fixed #7058 with this. 🙂

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.

None yet

3 participants