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

Move GHA prereqs to examples and refactor #321

Merged
merged 33 commits into from
Oct 15, 2024

Conversation

nicolecheetham
Copy link
Collaborator

@nicolecheetham nicolecheetham commented Oct 2, 2024

  • Moves the binary install prereqs to the necessary example directories
  • Removes unnecessary dependencies within each version of the GHA prereqs

Issue: #309

Issue: #61


This change is Reviewable

@nicolecheetham nicolecheetham marked this pull request as draft October 2, 2024 20:15
@jwnimmer-tri
Copy link
Contributor

FYI The setup scripts in each project like drake_cmake_installed/setup/foo should not be hard-coding their subdir name like drake_cmake_installed anywhere inside the script. When the users makes their own repo, that name will no longer match. The code that calls those scripts should chdir to the correct relative path (the subdir, not the git repository root) before calling anything.

@nicolecheetham nicolecheetham marked this pull request as ready for review October 7, 2024 16:40
Copy link
Collaborator Author

@nicolecheetham nicolecheetham left a comment

Choose a reason for hiding this comment

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

+@BetsyMcPhail for feature review please

Reviewable status: all discussions resolved, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)

@jwnimmer-tri jwnimmer-tri self-assigned this Oct 7, 2024
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail and @nicolecheetham)


drake_ament_cmake_installed/.github/ros_humble_setup line 11 at r2 (raw file):

export DEBIAN_FRONTEND='noninteractive'

cd "$(dirname "${BASH_SOURCE}")"

BTW I imagine all of these CI scripts would be easier to understand if the chdir had the .. part on it, instead of tacking it into the below:

cd $(dirname "${BASH_SOURCE}")/..
setup/ros_humble/binary_install_prereqs --ros-humble

drake_bazel_download/setup/ubuntu/binary_install_prereqs line 3 at r2 (raw file):

#!/bin/bash

# Copyright (c) 2020, Massachusetts Institute of Technology.

Working

These files being copied now (coming from scripts/setup/linux/ubuntu/jammy) are the last remnants of the license text spam we're trying to get rid of in DEE.

Give me a little time to see if there is any way we can avoid copying this spam, before we make it worse with this PR.


drake_ament_cmake_installed/setup/ros_humble/binary_install_prereqs line 1 at r2 (raw file):

#!/bin/bash

BTW What if the name of this script was always setup/install_prereqs instead of setup/ros_humble/binary_install_prereqs.

Drake's upstream setup logic at https://github.com/RobotLocomotion/drake/tree/master/setup is quite complicated so it split into subdirs & etc, but for these examples our goal is to keep it simple, so having extra directory nesting seems contrary to the goal?

Using a simple, uniform name, also lets us keep the file_sync_test intact.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 9 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @jwnimmer-tri and @nicolecheetham)


drake_ament_cmake_installed/setup/ros_humble/binary_install_prereqs line 72 at r2 (raw file):

tar -xf drake.tar.gz -C /opt

# Show version for debugging; use echo for newline / readability.

BTW I'm not sure if this should be removed? It's perhaps useful to show the drake version in the output?

Copy link
Collaborator Author

@nicolecheetham nicolecheetham left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail and @jwnimmer-tri)


drake_ament_cmake_installed/.github/ros_humble_setup line 11 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I imagine all of these CI scripts would be easier to understand if the chdir had the .. part on it, instead of tacking it into the below:

cd $(dirname "${BASH_SOURCE}")/..
setup/ros_humble/binary_install_prereqs --ros-humble

I agree. I will change it.


drake_bazel_download/setup/ubuntu/binary_install_prereqs line 3 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

These files being copied now (coming from scripts/setup/linux/ubuntu/jammy) are the last remnants of the license text spam we're trying to get rid of in DEE.

Give me a little time to see if there is any way we can avoid copying this spam, before we make it worse with this PR.

Okay.


drake_ament_cmake_installed/setup/ros_humble/binary_install_prereqs line 1 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW What if the name of this script was always setup/install_prereqs instead of setup/ros_humble/binary_install_prereqs.

Drake's upstream setup logic at https://github.com/RobotLocomotion/drake/tree/master/setup is quite complicated so it split into subdirs & etc, but for these examples our goal is to keep it simple, so having extra directory nesting seems contrary to the goal?

Using a simple, uniform name, also lets us keep the file_sync_test intact.

That could be possible. The one exception is drake_cmake_installed as the mac and ubuntu install_prereqs need to be distinguished from one another. This can be done with subdirs. Another way is renaming the multiple install_prereqs of this example to mac_install_prereqs and ubuntu_install_prereqs. Is there an option that is preferrable to you? I'm also open to other ideas.


drake_ament_cmake_installed/setup/ros_humble/binary_install_prereqs line 72 at r2 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

BTW I'm not sure if this should be removed? It's perhaps useful to show the drake version in the output?

I can add it back. It doesn't hurt to have something for debugging. Moreover, I was on the fence whether to remove it or not.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail and @nicolecheetham)


drake_ament_cmake_installed/setup/ros_humble/binary_install_prereqs line 1 at r2 (raw file):

Previously, nicolecheetham (Nicole C.) wrote…

That could be possible. The one exception is drake_cmake_installed as the mac and ubuntu install_prereqs need to be distinguished from one another. This can be done with subdirs. Another way is renaming the multiple install_prereqs of this example to mac_install_prereqs and ubuntu_install_prereqs. Is there an option that is preferrable to you? I'm also open to other ideas.

There is another option: just have one bash script called install_prereqs, but inside it has if-then-else blocks that check for which platform it's running on, and take different steps accordingly. That makes the user README even more clear -- run one script, no matter which platform you are on.

Copy link
Collaborator Author

@nicolecheetham nicolecheetham left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail and @jwnimmer-tri)


drake_ament_cmake_installed/setup/ros_humble/binary_install_prereqs line 1 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

There is another option: just have one bash script called install_prereqs, but inside it has if-then-else blocks that check for which platform it's running on, and take different steps accordingly. That makes the user README even more clear -- run one script, no matter which platform you are on.

That is a good idea. I'll work on it.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 9 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)


drake_bazel_download/setup/ubuntu/binary_install_prereqs line 3 at r2 (raw file):

Previously, nicolecheetham (Nicole C.) wrote…

Okay.

Retracted. Looking in more detail, yes I will be able to fix this, but it will be much easier after this PR moves the files around. For now, copying them with this blob intact is good.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 9 files at r3, 4 of 4 files at r5, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail and @nicolecheetham)


.github/workflows/ci.yml line 70 at r5 (raw file):

          pip3.11 --version
      - name: setup
        run: ./drake_cmake_installed/setup/install_prereqs --mac

The user should not need to pass a flag. (If they did, we would need to document that in the README, but that's not ideal.) The script itself can interrogate which OS it's running on, internally, and take the correct actions automatically.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 9 files at r3, 4 of 4 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @nicolecheetham)

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee betsymcphail, platform LGTM from [jwnimmer-tri] (waiting on @nicolecheetham)

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee betsymcphail, platform LGTM from [jwnimmer-tri] (waiting on @nicolecheetham)

@jwnimmer-tri
Copy link
Contributor

This is only waiting on an LGTM from @BetsyMcPhail.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r6, 3 of 4 files at r7, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [jwnimmer-tri, betsymcphail] (waiting on @nicolecheetham)

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r7.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [jwnimmer-tri, betsymcphail] (waiting on @nicolecheetham)

@BetsyMcPhail BetsyMcPhail merged commit 7147614 into RobotLocomotion:main Oct 15, 2024
9 checks passed
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.

3 participants