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

added support for chruby #6301

Closed
wants to merge 1 commit into from
Closed

added support for chruby #6301

wants to merge 1 commit into from

Conversation

tcurdt
Copy link
Contributor

@tcurdt tcurdt commented Sep 26, 2016

Thanks for contributing to fastlane! Before you submit your pull request, please make sure to check the following boxes:

  • Run rspec for all tools you modified
  • Run rubocop -a to ensure the code style is valid
  • We currently don't accept new actions, please publish a plugin instead, more information in Plugins.md

Before submitting a pull request, we appreciate if you create an issue first to discuss the change 👍 ... This is related to

#5240
#6277

and various other issues potential "device not found" issues.

Anyway - this is what finally fixed many issues for me. Would be great to get this merged.

(Who would have thought that the xcodebuild has a dependency to system ruby?!)

@KrauseFx
Copy link
Member

Thanks for your pull request @tcurdt. I don't have a lot of knowledge around this. @nafu Would that solve the issue for you? (via #6277 (comment))

@tcurdt
Copy link
Contributor Author

tcurdt commented Sep 26, 2016

It did solve a big issue for me - if that counts :)

@dflems
Copy link

dflems commented Oct 4, 2016

chruby is a function, not a binary. Since xcbuild-safe.sh is run in a subshell, I don't think it has access to functions declared outside of it unless sourced-in directly. I still think my comments in #5240 are relevant to this discussion:

RVM is the invasive one that needs special logic. rbenv and chruby just need the ruby environment unset. Aside from the RVM logic, this should work fine as a catch-all for other ruby managers (at least it's worked for me with chruby).

unset BUNDLE_BIN_PATH
unset BUNDLE_GEMFILE
unset GEM_HOME
unset GEM_PATH
unset RUBYLIB
unset RUBYOPT
unset RUBY_ENGINE
unset RUBY_ROOT
unset RUBY_VERSION
unset _ORIGINAL_GEM_PATH

@tcurdt
Copy link
Contributor Author

tcurdt commented Oct 4, 2016

@dflems I am not sure I am getting your point.

This is how one includes chruby:

source /usr/local/opt/chruby/share/chruby/chruby.sh
source /usr/local/opt/chruby/share/chruby/auto.sh
chruby ruby-2.3

Because it's function it is type -t chruby.

I also don't see a good reason why one should use a custom way of unsetting the ruby env. It's the contract of chruby to restore the env - why would we want to duplicate that?

@dflems
Copy link

dflems commented Oct 5, 2016

@tcurdt:

chruby is added by sourcing a few scripts in your shell and it becomes available as a function in your shell. We shouldn't make the expectation that that function will be available anywhere that xcbuild-safe.sh is called. It would be one thing if chruby was a binary and lived on the PATH, but I feel it's unwise to make the assumption that the function will be available in all cases.

For instance, this doesn't work for me because I use zsh and source the chruby scripts into my .zshrc. The shebang in xcbuild-safe.sh is #!/bin/bash --login which means it runs bash with a login shell. If you use bash, this may work fine for you. Since I use zsh and don't source the chruby scripts into my bash profile, this will not work for me (and potentially many other people who use zsh primarily and don't keep bash in sync with it). Do you follow?

Unsetting the ruby-specific environment variables will cover the general use-case for chruby and rbenv across-the-board (and maybe RVM too). We might be able go go even further by just obliterating anything set in the user shell and starting fresh:

#!/usr/bin/env -i sh
# ^ starts a fresh shell with no environment

# source system-wide profile
source /etc/profile

# xcode should not be polluted :D
xcodebuild "$@"

EDIT: The above solution is actually no good because it strips ALL environment variables. There are some that we might want to make it through to xcodebuild (NSUnbufferedIO comes to mind)

@tcurdt
Copy link
Contributor Author

tcurdt commented Oct 5, 2016

I am using a .profile and no .bashrc but that might be beside the point.

Right now the assumption is that if there is a chruby function the ruby version could potentially be non-system and if there is no chruby function it is safe to assume it's system ruby (or at least not altered by chruby). If that assumption is wrong you might have a point.

I don't understand your scenario yet though. If you use chruby from zsh - but the xcbuild-safe.sh uses bash (via shebang) - why should you have anything but the sytem ruby in your bash?

Resetting the full env sounds like an easy approach on the first glance - but users might expect env variables to be there that suddenly aren't. I am not convinced this will turn out to be the best approach in real life.

@dflems
Copy link

dflems commented Oct 5, 2016

@tcurdt: Even if the shebang specifies that it should run the bash login shell, the script will still inherit the environment from its parent process (in this case fastlane which itself inherits from the shell that's executing it). It won't have a pristine environment, so GEM_HOME, etc. from my shell will make it through that script.

Functions are not inherited, especially across shells, so in my case, xcbuild-safe.sh will have all of the ruby environment variables set from my shell but they won't get unset because it won't be aware that the chruby function exists. I also don't think this is some weird corner case. Your solution works well for your situation but will not work for everyone.

@milch
Copy link
Collaborator

milch commented Nov 15, 2016

After looking at the code and the discussion, I agree with @dflems that this is a very specific solution that probably wouldn't work for most users. I'm closing this for now, as the underlying issues have been resolved (see 5240#issuecomment-253082825) - that should affect most users.

If there is still an issue, let us know and we can reopen the discussion! 🚀

@milch milch closed this Nov 15, 2016
@fastlane fastlane locked and limited conversation to collaborators Feb 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants