Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Stop deleting the prefix path on install of TruffleRuby #1783

Closed

Conversation

casperisfine
Copy link

This is essentially a revert of cda9b11

I understand the intent of helping people using ruby-build for their dev environment, where multiple rubies coexists in multiple sub directories, but:

  • MRI recipes don't behave like that, so it's surprising.
  • In my case I ruby-build to generate production Docker images, so I directly install ruby with /usr/local as a prefix, and this delete important, unrelated files.

So I believe it does more harm than good.

cc @eregon
cc @tomstuart @chrisseaton

@eregon
Copy link
Member

eregon commented Jul 26, 2021

I think I would rather abort in clean_prefix_path if the prefix is /usr or /usr/local or not-a-Ruby-prefix.
The criteria for a-Ruby-prefix could be $dir does not exist, or $dir/bin/ruby exists.

Essentially TruffleRuby does not support being installed in /usr/local currently: oracle/truffleruby#1389

I think installing software in /usr/local (directly, not in subdirectory) is a strange practice, it's impossible to uninstall, it's impossible to clean up or reinstall, etc. That's based on my experience, but maybe others have different views on this?
Also for Ruby in general it means gem install won't work as it would try to write to /usr/local which is not writable (except by root, and running things as root -- even in Docker -- seems generally bad practice).
I understand this might matter a bit less for Docker, but still it's mixing random files together and there is a not-so-low chance of conflicts or things going wrong due to that. And if one ever wants to debug that image and e.g. reinstall TruffleRuby inside it, it would be very difficult.

@eregon
Copy link
Member

eregon commented Jul 26, 2021

Another view of this, if the software is packaged as a prebuilt archive like TruffleRuby and JRuby, how does it feel if you would have to manually extract it to /usr/local? I feel that would be extremely messy, and a good illustration of how I feel about installing into /usr/local.

@casperisfine
Copy link
Author

Essentially TruffleRuby does not support being installed in /usr/local currently: oracle/truffleruby#1389

Weird, after patching ruby-build everything seemed to work. I'll read that issue further.

I think installing software in /usr/local (directly, not in subdirectory) is a strange practice, it's impossible to uninstall, it's impossible to clean up or reinstall, etc. That's based on my experience, but maybe others have different views on this?

Yes. In our case we use Docker layers and rebuild from scratch. Re-install or uninstall is not a concern, and installing in /usr/local makes things simpler as we don't have to add to the $PATH.

Also for Ruby in general it means gem install won't work as it would try to write to /usr/local which is not writable (except by root, and running things as root -- even in Docker -- seems generally bad practice).

Not a concern either for out use case, it's almost a feature as we ensure dependencies are only handled by bundler.

@casperisfine
Copy link
Author

how does it fell if you would have to manually extract it to /usr/local? I feel that would be extremely messy, and a good illustration of how I feel about installing into /usr/local.

Isn't that how most package managers work? .deb or .rpm packages are essentially what you described AFAIK.

I feel that would be extremely messy

What would be very messy IMHO would be to have my entire /usr/local nuked if I attempted to do what I did on my machine. I'm very glad I was working with Docker images...

@casperisfine
Copy link
Author

installing in /usr/local makes things simpler as we don't have to add to the $PATH.

To clarify this statement after reading oracle/truffleruby#1389, our system build docker images dynamically. So we have a bunch of code that expect "some ruby" to be installed in /usr/local/bin/ruby, but dynamically select which one and install it with ruby-build, e.g. to simplify:

$ ruby-build "${USER_PROVIDED_RUBY_VERSION}" /usr/local/

@eregon
Copy link
Member

eregon commented Jul 26, 2021

Isn't that how most package managers work? .deb or .rpm packages are essentially what you described AFAIK.

There are a couple important differences for package managers:

  • package managers know exactly which files are installed for a package, and how to remove them
  • The list of files is carefully reviewed to ensure no conflict, etc
  • There is actually some handling of configuration or other files which are created later when using the software

So we have a bunch of code that expect "some ruby" to be installed in /usr/local/bin/ruby, but dynamically select which one and install it with ruby-build, e.g. to simplify:

Is the requirement just to have ruby in PATH, or is /usr/local/bin/ruby actually hardcoded somewhere?
Changing the PATH in Docker is really easy.

@casperisfine
Copy link
Author

Is the requirement just to have ruby in PATH, or is /usr/local/bin/ruby actually hardcoded somewhere?

ruby in path.

Changing the PATH in Docker is really easy.

I know, but this rm -rf, which is a radical difference in behavior from the MRI definitions, really threw me off, hence why I opened this PR rather than change code that worked for a long time with MRI definitions.

But if you are adamant this rm -rf must stay, then of course I'll work around it. But I'd rather check first.

@eregon
Copy link
Member

eregon commented Jul 26, 2021

But if you are adamant this rm -rf must stay, then of course I'll work around it. But I'd rather check first.

I think it should stay at least for a-Ruby-prefix cases.
It makes reinstallation so much more reliable and I value that a lot (especially for truffleruby-dev).

For not-a-Ruby-prefix cases, I agree the rm -rf is not OK and I am sorry about that.
I think an error is best there with TruffleRuby.
Just skipping the rm would likely lead to other issues, so I'm not really happy about that.

I'll make an alternative PR.

@casperisfine
Copy link
Author

I'll make an alternative PR.

👍

@eregon
Copy link
Member

eregon commented Jul 26, 2021

=> #1784

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants