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

Fork of Drake's install_prereqs.sh package list is overly brittle #61

Open
jwnimmer-tri opened this issue Nov 1, 2017 · 24 comments
Open
Assignees

Comments

@jwnimmer-tri
Copy link
Contributor

jwnimmer-tri commented Nov 1, 2017

https://github.com/RobotLocomotion/drake-external-examples/blob/master/scripts/setup/linux/ubuntu/xenial/install_prereqs duplicates Drake's list of dependencies. This is a maintenance burden. We need to find a way to allow for everyday dependency changes to Drake without PR'ing to two separate places.

Conceivably, more intricate changes such as bumping Bazel, or changing the compiler, could have duplicated logic. But "Add system libfoo-dev" should not require a separate PR here.

@stonier
Copy link
Contributor

stonier commented Nov 1, 2017

Yes, already noted, but thanks for making it an issue.

@nuclearsandwich
Copy link
Contributor

nuclearsandwich commented Nov 9, 2017

One option would be to have drake-external-examples's setup try to curl Drake's from GitHub. Could even get fancy with branch logic if you wanted.

#!/usr/bin/env bash

set -o errexit
set -o pipefail

DRAKE_BRANCH="${1:-master}"
PLATFORM="ubuntu/16.04"

DRAKE_INSTALL_PREREQS_URL="https://raw.githubusercontent.com/RobotLocomotion/drake/${DRAKE_BRANCH}/setup/${PLATFORM}/install_prereqs.sh"

if [[ ! $(which curl > /dev/null 2>&1) ]]; then
	apt-get update && apt-get install -y curl
fi

echo "Fetching Drake prereqs script."
curl "$DRAKE_INSTALL_PREREQS_URL" > drake-install-prereqs.sh
bash drake-install-prereqs.sh

It's just a mock-up, not comprehensive or robust, but one possible option.

@stonier
Copy link
Contributor

stonier commented Nov 9, 2017

I was thinking of having some install script installed by Drake so it can be used within and without. It probably would be a subset of what a Drake bazel workspace needs as well.

@stonier
Copy link
Contributor

stonier commented Nov 9, 2017

I believe the present solution was just a temporary copy/paste from jamie to get CI on board.

@jamiesnape
Copy link
Contributor

Yes, we are going to install a version of the install_prereqs.sh script with the binary packages, which contains the subset of packages needed by users of those packages.

@jamiesnape jamiesnape changed the title Shabhala's fork of Drake's install_prereqs.sh package list is overly brittle Fork of Drake's install_prereqs.sh package list is overly brittle Feb 8, 2019
@EricCousineau-TRI
Copy link
Collaborator

Given the re-breakage to be fixed by #142, perhaps we slate this to be worked on in early Dec?

@jwnimmer-tri
Copy link
Contributor Author

I broke drake-external-examples master again because of this via https://reviewable.io/reviews/RobotLocomotion/drake/12680. Drake's package list of source dependencies changed, but I failed to copy that change into this repository.

@jwnimmer-tri
Copy link
Contributor Author

Another hiccup in #177.

@BetsyMcPhail
Copy link
Contributor

This came up again with changes in RobotLocomotion/drake#15618

@RussTedrake
Copy link
Contributor

RussTedrake commented Aug 23, 2021

I've been calling drake's installed install_prereqs transitively from the underactuated / manipulation install_prereqs for a long time. Am I to understand that everyone agrees this is simple to do here, but just nobody has taken the time to do it?

@jwnimmer-tri
Copy link
Contributor Author

That is probably true. I've added this issue to the Build & Release project board now.

The only reason I say "probably" here is that we not only want to make the CI jobs here run more smoothly, but also we want to make sure this example repository is what we actually want to recommend to end-users. As we develop any patch towards this issue, we'll want to make sure to keep that second use case in mind.

For example, in Anzu, we do not transitively call Drake's install_prereqs. Instead, we pin Drake and pin its prereq packages, and update the two in lockstep on purpose. This can help because the packages-bionic-test-only.txt are not necessarily required for external users, though perhaps transitively calling using --without-test-only would suffice. Also, things like clang-9 are in the non-test source prereqs, but are only required when building using one of our Clang variants (like a sanitizer). It's not clear that we want Downstream projects to use that. It's possible that the answer to all of these is that "those are Drake bugs; the example should always just transitive call Drake's setup script", but even in that case we should be sure to file the Drake bugs as we prepare and review these changes.

For the "use Drake via a binary release" variants in this repository, transitively calling the binary's install_prereqs is probably best. For "rebuild Drake from source", it might be more subtle (or not).

@RussTedrake
Copy link
Contributor

As I was most of the way through reading your third paragraph, I was indeed thinking "those sound like Drake bugs". :-)

@BetsyMcPhail
Copy link
Contributor

@alesgenova this issue might be good for you to look at

@alesgenova alesgenova removed their assignment Mar 3, 2022
@jwnimmer-tri
Copy link
Contributor Author

We should finish #219 before starting this one.

@alesgenova alesgenova self-assigned this Mar 14, 2022
@BetsyMcPhail BetsyMcPhail removed their assignment Mar 15, 2022
@jwnimmer-tri
Copy link
Contributor Author

To re-iterate the above, the following examples should shell out to drake/share/drake/setup/install_prereqs from whatever version of Drake's binaries that they have downloaded:

  • drake_bazel_installed
  • drake_cmake_installed
  • drake_catkin_installed
  • drake_ament_cmake_installed

For the apt-based examples, we should only need to apt install Drake -- the dependencies should come along as part of that install already, there should be no to additionally call install_prereqs:

  • drake_cmake_installed_apt

Let's fix the above ones before we dive into the from-source examples.

@EricCousineau-TRI
Copy link
Collaborator

Dunno if it helps / hurts, but as an interim we recently started doing a kind-of hacky "boostrappy Bazel" approach for drake-ros Bazel workflows:
https://github.com/RobotLocomotion/drake-ros/blob/a4d976d6ef093031b4a08e31446527c814882501/.github/workflows/bazelized_drake_ros.yml#L25-L30
This involves installing minimum deps to effectively enable something akin to bazel fetch @drake, then possibly overriding those deps (e.g. different Bazel) when Drake itself installs. Plus, it caches the repository download of Drake.

@BetsyMcPhail
Copy link
Contributor

To re-iterate the above, the following examples should shell out to drake/share/drake/setup/install_prereqs from whatever version of Drake's binaries that they have downloaded:

  • drake_bazel_installed
  • drake_cmake_installed
  • drake_catkin_installed
  • drake_ament_cmake_installed

For the apt-based examples, we should only need to apt install Drake -- the dependencies should come along as part of that install already, there should be no to additionally call install_prereqs:

  • drake_cmake_installed_apt

Let's fix the above ones before we dive into the from-source examples.

@jwnimmer-tri it's been awhile since I looked at this issue, is the above still a fair place to start with this work?

@jwnimmer-tri
Copy link
Contributor Author

Yes, those are good outcomes to aim for.

The related point is that the supporting glue code at https://github.com/RobotLocomotion/drake-external-examples/tree/main/scripts is not particularly very well factored, and we aim to eventually rewrite it to be simpler. So in case the organization of those scripts is getting in the way of this work, then it's OK to make more-than-minor changes to them in case that makes things easier.

@nicolecheetham
Copy link
Collaborator

@jwnimmer-tri The drake-external-examples ubuntu install_prereqs seems to already call drake's version via /opt/drake/share/drake/setup/install_prereqs on line 75. If this is supposed to grab all the prereqs drake needs while on ubuntu then why is the DEE examples installing more packages? A good portion of these packages are exactly the same as the ones that should be installed via drake's install script. Also, why is the shell out to drake/share/drake/setup/install_prereqs? Even if one just goes by the latter drake directory, there are no install_prereqs files in setup. One would first need to cd into the correct system folder first then one would see the install_prereqs.

@nicolecheetham
Copy link
Collaborator

Follow Up: Couldn't I just follow drake's source installation page to clone and run the install prereqs instead of using the drake external example install_repreqs?

@jwnimmer-tri
Copy link
Contributor Author

Couldn't I just follow drake's source installation page to clone and run the install prereqs instead of using the drake external example install_repreqs?

The first goal of this issue is to only fix the binary installation workflows, and not change anything related to source installation workflows.

To be specific, we should check / change / fix these:

  • drake_ament_cmake_installed
  • drake_bazel_download
  • drake_catkin_installed
  • drake_cmake_installed
  • drake_cmake_installed_apt

We should not change these:

  • drake_bazel_external
  • drake_cmake_external

The drake-external-examples ubuntu install_prereqs seems to already call drake's version via /opt/drake/share/drake/setup/install_prereqs on line 75.

Right.

If this is supposed to grab all the prereqs drake needs while on ubuntu then why is the DEE examples installing more packages?

The call to action in this issue is that it should not be doing that.

The parts like "ament" and "catkin" are things needed by the DEE example code rather than by drake itself, so it is OK to still install those, but they should only happen for the flavors that need them, and (ideally) that little install script would live in e.g. drake_catkin_installed/install_prereqs.sh or something like it so that it's co-located with the subdir that needs it, and much clearer to users who are reading the example.

Also, why is the shell out to drake/share/drake/setup/install_prereqs? Even if one just goes by the latter drake directory, there are no install_prereqs files in setup.

In the binary release like https://github.com/RobotLocomotion/drake/releases/download/v1.32.0/drake-1.32.0-jammy.tar.gz there are such files. Don't confuse the git source tree layout with the binary install media layout.

@nicolecheetham
Copy link
Collaborator

nicolecheetham commented Sep 6, 2024

@jwnimmer-tri So, if I am understanding what you said correctly:

Would I not change anything with the Jenkinsfile workflow yet as it deals with the externals and thus source installation?

Additionally, since both the Jenkinsfile and Github Actions ultimately use the ubuntu jammy install_prereqs, the install_prereqs currently gets all distributions (source, binary, and some others too) to correctly run the external workflows and satisfy all build tests. The non-externals do not need the source distributions so refactoring of the install_prereqs is necessary. Refactoring of the directories can also take place to reduce directory confusion and remove the existence of a paper trail for the install_prereqs.

An approach to simplify/refactor the workflow would be to create a separate install_prereqs for the internals which just need the binary distributions and any additional packages for the DEE examples. A first step to this would essentially split the ubuntu_jammy install_prereqs into 2; one for the Jenkinsfile and one for the Github Actions.

As for further compartmentalizing the install_prereqs with ament and catkin, one could have some if logic deciding when they are necessary and then call the separate files that house the distribution installations (of which are currently in the install_prereqs file).

@jwnimmer-tri
Copy link
Contributor Author

Would I not change anything with the Jenkinsfile workflow yet as it deals with the externals and thus source installation?

Looking at the table here, the os in the table claim that Jenkins runs drake_bazel_download and drake_catkin_installed and drake_cmake_installed which are in scope for fixing in this ticket (per the above list). And indeed, looking at recent logs does show those projects being run in Jenkins.

If different flavors are all being run back-to-back on the same machine, then their varied install_prereqs will pollute each other. Not good. If that's true, then we'll also need to adjust the Jenkins logic so that each sub-project is run independently.

Or, possibly a better tactic would be that each sub-project (drake_bazel_download, drake_cmake_installed, etc.) is run in exactly one of either GitHub Actions xor Jenkins. I don't remember any reason why Jenkins should be duplicating GHA, so we should probably just kill the Jenkins flavors that overlap. The only thing to check first is that GHA is set up to run (i.e., trigger) every night on a timer. We want to find build problems no later than the next day, so we need every sub-project to run nightly.

Refactoring of the directories can also take place to reduce [confusion].

Yes.

Additionally, since both the Jenkinsfile and Github Actions ultimately use the ubuntu jammy install_prereqs, the install_prereqs currently gets all distributions (source, binary, and some others too) to correctly run the external workflows and satisfy all build tests. The non-externals do not need the source distributions so refactoring of the install_prereqs is necessary.

I haven't traced through the scripts, but if the premise in the first part of your paragraph is correct, then use the consequent is also correct -- we can and should refactor the code so each individual project only runs.

@nicolecheetham
Copy link
Collaborator

nicolecheetham commented Sep 10, 2024

@jwnimmer-tri Okay, thank you for the clarification on the Jenkinsfile. I have a couple more questions to ask now.

Do flavors represent different builds or whether a test uses binary or source distributions? If the former, then these flavors are being run back-to-back for both the GHA and Jenkins when using ubuntu jammy. The paths for these files are scripts/continuous_integration/{jenkins, github_actions}/build_test.

Each os has a singular "build and test" step with ubuntu jammy section executing multiple builds. I believe that I could resolve the back-to-back running by isolating the build tests in GHA and Jenkinsfile. I can also look into each build test running once if GHA runs every night.

And could you elaborate on what you mean by "each individual project only runs"? My current understanding is that there should be two install_preqs. One for the binary builds and one for the source/external builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

9 participants