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 gcc and cc for darwin (no cli tools installed) #941

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@erikng
Copy link
Contributor

erikng commented Jan 15, 2017

If darwin is found:

  1. shell out to /usr/bin/xcode-select -p
    2a. If it returns an exit code other than 0, assume xcode command line tools are not installed and do not run these commands.
    2b. If it returns an exit code of 0, run the commands.

Signed-off-by: Erik Gomez e@eriknicolasgomez.com

Description

This removes the requirement for both Ohai and Chef to have the Xcode command line tools installed on macOS machines.

This is a huge win for managing client devices as the Xcode Command Line tools were previously a dependency. Companies have worked around these issues, but they are not ideal and require additional management overhead/engineering effort.

One such example: https://github.com/facebook/IT-CPE/blob/master/chef/tools/chef_bootstrap.py#L239-L284

Issues Resolved

Fixes #940

Check List

Since this only impacts Darwin, I have tested both use cases where xcode command line tools are installed and when they are not installed.

fix gcc and cc for darwin (no cli tools installed)
If darwin is found:
1. shell out to /usr/bin/xcode-select -p
2a. If it returns an exit code other than 0, assume xcode command line tools are not installed and do not run these commands.
2b. If it returns an exit code of 0, run the commands.

Signed-off-by: Erik Gomez <e@eriknicolasgomez.com>
@natewalck

This comment has been minimized.

Copy link
Contributor

natewalck commented Jan 15, 2017

This would be an amazing fix to get merged. Obviously the tests must be updated, but is the overall form acceptable? (CC @jaymzh @mcquin @tas50 )

@erikng

This comment has been minimized.

Copy link
Contributor

erikng commented Jan 15, 2017

To be clear, the integration tests are probably failing due to the current code and not my changes as the CI tools do not run on macOS.

@thommay

This comment has been minimized.

Copy link
Collaborator

thommay commented Jan 16, 2017

@erikng the tests are failing due to expectations in the specs that are no longer true, so you'll need to fix those up. Chefstyle also has some errors that you'll need to fix (run bundle exec rake style:auto_correct).

In general though, I think each check ought to be more split out. For darwin, linux, etc, the checks that matter are gcc and glibc, so we could wrap those in a function, and then do something like:

collect_data(:darwin) do
  if xcode_installed
    collect_gcc
    collect_glibc
  end
end

collect_data(:windows) do
  # check for cl
  # check for devenv.com
end

collect_data(:default) do
  collect_gcc
  collect_glibc
end

If that seems like a too major refactor, that's ok, just let us know and we can take it on from here (but please tick the box to "Allow edits from maintainers" on this PR - https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/ )

@erikng

This comment has been minimized.

Copy link
Contributor

erikng commented Jan 16, 2017

I would certainly be willing to do it for the Darwin side, but as this PR was designed to fix the specific issues presented, I had no intention of doing other operating systems.

If you were to do the refactor, what are the timetables here? This would infinitely make our deployments easier and I would hate for other factors to delay this fix.

refactor c.rb based on PR feedback
Signed-off-by: Erik Gomez <e@eriknicolasgomez.com>
@erikng

This comment has been minimized.

Copy link
Contributor

erikng commented Jan 16, 2017

I went ahead and completely refactored it, based on the feedback you gave @thommay (wasn't as bad as I expected) and the Chefstyle is no longer producing errors.

That said, I have very little ideas on how to fix the spec tests, especially now. Some feedback would be greatly appreciated.

erikng added some commits Jan 16, 2017

fixed Chefstyle
Signed-off-by: Erik Gomez <e@eriknicolasgomez.com>
move functions to top
Signed-off-by: Erik Gomez <e@eriknicolasgomez.com>
add colons back to debug log
Signed-off-by: Erik Gomez <e@eriknicolasgomez.com>
@tas50

This comment has been minimized.

Copy link
Member

tas50 commented on ab4de09 Jan 17, 2017

Thanks

@erikng

This comment has been minimized.

Copy link
Contributor

erikng commented Jan 17, 2017

No problem. I've tried various things today to understand the spec, but I'm not getting any closer to writing it. If someone could give me a little feedback on where to start here it would be appreciated, otherwise I may not be able to finish this PR.

I would hate to close it, but it may be infinitely easier for me to just deploy this as a hotfix (to our macOS devices) rather than spend time I don't have fixing the specs.

If I sound bitter, I apologize, but I've wasted about 4 hours of time today and gotten almost zero progress.

@erikng

This comment has been minimized.

Copy link
Contributor

erikng commented Jan 23, 2017

Closing due to #944

@erikng erikng closed this Jan 23, 2017

@chef chef locked and limited conversation to collaborators Nov 16, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.