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

Implement support for options with homebrew. #304

Merged

Conversation

NikolausDemmel
Copy link
Contributor

  • The support for homebrew options seems to have been lost in fa4d0b6.
  • We support installing options and checking installed options.
  • Options are accepted if the installed ones are a super set of the ones
    declared in rosdep.
  • Checking installed packages can be slow, as we have to call the brew
    command for each potential package to check if it is really linked and if the
    options are ok.
  • There are some limitations to what we can detect, for example we do not check
    for formulae from wrong taps, we do not preserve already installed options
    for reinstall of packages (see TODOs in source).
  • Some scenarios cannot be resolved automatically, e.g. installing the correct
    options for a package that is also in the dependency list of another homebrew
    package to be installed (see TODOs in source).
  • See Check version and options of installed formulas (OSX, Homebrew) #299 for a discussion
    of what the semantics should/could be.
  • This is an initial suggestion of what we could implement.
  • What is missing is updating the unit tests.

@NikolausDemmel
Copy link
Contributor Author

CI fails as the unit tests are not updated.

Moreover the rosdep rules in https://github.com/ros/rosdistro/blob/master/rosdep/osx-homebrew.yaml should be updated if this is merged / released, since the --universal options are probably unwanted on newer os x versions and the options for vtk should also be checked.

@NikolausDemmel
Copy link
Contributor Author

Apart from boost --with-python, here is another one where we might want to add an option: http:https://answers.ros.org/question/173654/cant-install-pcl-with-homebrew-on-osx-109-during-ros-hydro-installation/

However, in this case it is maybe more of an "install hint". I.e. if PCL is installed, we don't care what version it is (i.e. we don't enforce "HEAD"), but when it is not installed, we install --HEAD (on Mavericks).

@wjwwood
Copy link
Contributor

wjwwood commented Jun 24, 2014

So apart from updating the tests this makes sense to me. @NikolausDemmel do you think it is worth the effort to get this pushed through or no?

NikolausDemmel added a commit to NikolausDemmel/rosdistro that referenced this pull request Jun 24, 2014
 - they are ignored at the moment and therefore have not been used in a long time
 - on recent OS X these options are certainly wrong, so this is a prerequisite for merging
   ros-infrastructure/rosdep#304
NikolausDemmel added a commit to NikolausDemmel/rosdistro that referenced this pull request Jun 24, 2014
 - as long as ros-infrastructure/rosdep#304
   is not merged these will be ignored. If we merge this before rosdep
   is updated it will make testing that rosdep change slightly more easy.
 - install_flags will be used for `brew install`, but unlike options
   they are not checked for already installed packages
NikolausDemmel added a commit to NikolausDemmel/rosdistro that referenced this pull request Jun 24, 2014
 - as long as ros-infrastructure/rosdep#304
   is not merged these will be ignored. If we merge this before rosdep
   is updated it will make testing that rosdep change slightly more easy.
 - install_flags will be used for `brew install`, but unlike options
   they are not checked for already installed packages
 - for boost --with-python see ros-infrastructure/rosdep#330
 - for pcl --HEAD on Mavericks see
   - http:https://answers.ros.org/question/173654/cant-install-pcl-with-homebrew-on-osx-109-during-ros-hydro-installation/
   - ros/homebrew-hydro#3
NikolausDemmel added a commit to NikolausDemmel/rosdistro that referenced this pull request Jun 24, 2014
 - as long as ros-infrastructure/rosdep#304
   is not merged these will be ignored. If we merge this before rosdep
   is updated it will make testing that rosdep change slightly more easy.
 - install_flags will be used for `brew install`, but unlike options
   they are not checked for already installed packages
 - for boost --with-python see ros-infrastructure/rosdep#330
 - for pcl --HEAD on Mavericks see
   - http:https://answers.ros.org/question/173654/cant-install-pcl-with-homebrew-on-osx-109-during-ros-hydro-installation/
   - ros/homebrew-hydro#3
NikolausDemmel added a commit to NikolausDemmel/rosdistro that referenced this pull request Jun 25, 2014
 - as long as ros-infrastructure/rosdep#304
   is not merged these will be ignored. If we merge this before rosdep
   is updated it will make testing that rosdep change slightly more easy.
 - install_flags will be used for `brew install`, but unlike options
   they are not checked for already installed packages
 - for boost --with-python see ros-infrastructure/rosdep#330
 - for pcl --HEAD on Mavericks see
   - http:https://answers.ros.org/question/173654/cant-install-pcl-with-homebrew-on-osx-109-during-ros-hydro-installation/
   - ros/homebrew-hydro#3
 - The support for homebrew options seems to have been lost in fa4d0b6.
 - This commit supports installing options and checking installed options.
 - Options are accepted if the installed ones are a super set of the ones
   declared in rosdep.
 - We also add support for `install_flags`, which are similar to options
   and get passed to `brew install`, but do not get checked for already
   installed packages.
 - Checking installed packages can be slow, as we have to call the `brew`
   command for each potential package to check if it is really linked and if the
   options are ok.
 - There are some limitations to what we can detect, for example we do not check
   for formulae from wrong taps, we do not preserve already installed options
   for reinstall of packages (see TODOs in source).
 - Some scenarios cannot be resolved automatically, e.g. installing the correct
   options for a package that is also in the dependency list of another homebrew
   package to be installed (see TODOs in source).
 - See ros-infrastructure#299 for a discussion
   of what the semantics should/could be.
@NikolausDemmel
Copy link
Contributor Author

@wjwwood: I think we can reuse some of this for xylem, so yes.

Latest commit has updated/passing tests. I also added install_flags to install --HEAD for PCL on Mavericks (this isn't an option in the homebrew sense).

Please review.

NikolausDemmel added a commit to NikolausDemmel/rosdistro that referenced this pull request Jun 26, 2014
 - Change from our mongodb-dev to homebrew maintained mongodb
 - homebrew maintained mongodb builds and installs libs since Homebrew/legacy-homebrew#25253
 - --with-boost option is needed, so depends on ros-infrastructure/rosdep#304

Is there a way to list all released pacakges depending on the `mongodb-dev` rosdep key?
return False
for spec in pkg_info['installed']:
if spec['version'] == linked_version:
# print(r.package, spec)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably drop this debugging line.

@wjwwood
Copy link
Contributor

wjwwood commented Jun 26, 2014

Other than my notes the code seems to be OK. I still need to test it locally.

@wjwwood
Copy link
Contributor

wjwwood commented Jun 26, 2014

This seems to work for me locally, +1

@tfoote or @dirk-thomas can we get a review from one of you too?

@wjwwood
Copy link
Contributor

wjwwood commented Jul 2, 2014

Sorry @NikolausDemmel we are really behind over here, I'm doing my best to get another review on this, and then I'll try to get it into a release.

I appreciate your help tracking and pushing this along.

@wjwwood
Copy link
Contributor

wjwwood commented Jul 3, 2014

Ok, I'm going to merge this, I'll plan to do a release today if I have time, otherwise it may be Monday before I get to it.

wjwwood added a commit that referenced this pull request Jul 3, 2014
Implement support for options with homebrew.
@wjwwood wjwwood merged commit 9e4aec2 into ros-infrastructure:master Jul 3, 2014
@NikolausDemmel
Copy link
Contributor Author

Thanks!

@wjwwood
Copy link
Contributor

wjwwood commented Jul 7, 2014

We were waiting on the farm to get sync'ed up before I did a release, @tfoote can I release now?

@wjwwood
Copy link
Contributor

wjwwood commented Jul 29, 2014

I guess I need to release rosdep again with this pull request. I was reinstalling everything with rosdep and boost didn't get the --with-python option.

@wjwwood
Copy link
Contributor

wjwwood commented Jul 29, 2014

0.10.29 is out now with this change.

@NikolausDemmel
Copy link
Contributor Author

thanks

@NikolausDemmel NikolausDemmel deleted the homebrew-check-options-patch branch August 8, 2014 11:06
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

2 participants