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

Rename .deb files to skip per-distro subdirectories in S3 #22471

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tyler-yankee
Copy link

@tyler-yankee tyler-yankee commented Jan 15, 2025

Issue: #19735

  • Change _push_deb in push_release.py to copy artifacts directly to the drake_release folder in S3, rather than the per-distro subdirectories. The platform name is hyphenated at the end of the filename.
  • Change push_release to copy the corresponding filename convention.

This change is Reviewable

Copy link
Author

@tyler-yankee tyler-yankee 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 BetsyMcPhail, missing label for release notes (waiting on @tyler-yankee)


a discussion (no related file):
+@BetsyMcPhail for feature review

@tyler-yankee tyler-yankee removed the request for review from BetsyMcPhail January 15, 2025 19:31
@mwoehlke-kitware
Copy link
Contributor

tools/release_engineering/dev/push_release.py line 357 at r2 (raw file):

    """
    for deb in state.find_artifacts(_Manifest.RE_DEB):
        dest_name = f'drake/release/drake-dev_{deb.version}_{deb.arch}-{deb.platform}.{deb.ext}'

Note, keeping this ≤ 79 chars is preferred. Maybe move some of the RHS to a temporary dest_suffix?

@mwoehlke-kitware
Copy link
Contributor

tools/release_engineering/dev/push_release.py line 357 at r2 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Note, keeping this ≤ 79 chars is preferred. Maybe move some of the RHS to a temporary dest_suffix?

Also, this is a path, not a file name; consider keeping dest_path and not having a dest_name.

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.

Reviewable status: 3 unresolved discussions, LGTM missing from assignees mwoehlke-kitware,BetsyMcPhail, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @tyler-yankee)


a discussion (no related file):

Previously, tyler-yankee wrote…

+@BetsyMcPhail for feature review

+@mwoehlke-kitware for feature review please


tools/release_engineering/dev/push_release line 107 at r2 (raw file):


    # Add the Debian package to the aptly database.
    aptly repo add "drake-${platform}" "${filename}"

@mwoehlke-kitware - Should the filename here should remain the same as it is now with no platform? For example, drake-dev_1.37.0-1_amd64.deb

@tyler-yankee
Copy link
Author

tools/release_engineering/dev/push_release.py line 357 at r2 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Also, this is a path, not a file name; consider keeping dest_path and not having a dest_name.

Thanks, agreed. Fixed in the most recent commit.

Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware 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 BetsyMcPhail, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @tyler-yankee)


a discussion (no related file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

+@mwoehlke-kitware for feature review please

:lgtm: aside from pending discussion on naming for aptly.


tools/release_engineering/dev/push_release line 107 at r2 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

@mwoehlke-kitware - Should the filename here should remain the same as it is now with no platform? For example, drake-dev_1.37.0-1_amd64.deb

Are you asking if we should remove the platform from the file prior to aptly repo add? I don't know... ask someone with more Debian packaging knowledge. 🙂


tools/release_engineering/dev/push_release.py line 359 at r3 (raw file):

        dest_path_suffix = f'{deb.version}_{deb.arch}-{deb.platform}.{deb.ext}'
        state.push_artifact(deb, _AWS_BUCKET,
                            f'drake/release/drake-dev_{dest_path_suffix}')

Consider?

Suggestion:

        dest_suffix = f'{deb.version}_{deb.arch}-{deb.platform}.{deb.ext}'
        dest_path = f'drake/release/drake-dev_{dest_suffix}'
        state.push_artifact(deb, _AWS_BUCKET, dest_path)

@tyler-yankee
Copy link
Author

tools/release_engineering/dev/push_release.py line 359 at r3 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Consider?

Yeah, looks like this is cleanest.

@tyler-yankee
Copy link
Author

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

:lgtm: aside from pending discussion on naming for aptly.

I also just updated the README so it contains the new filename.

@tyler-yankee
Copy link
Author

tools/release_engineering/dev/push_release line 107 at r2 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Are you asking if we should remove the platform from the file prior to aptly repo add? I don't know... ask someone with more Debian packaging knowledge. 🙂

I don't have knowledge here, but strictly speaking I feel like it should remain unchanged if we want to stick as close as possible to current behavior outside of the intended change (which this is).

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 1 files at r2, 1 of 2 files at r4, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee BetsyMcPhail, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @tyler-yankee)


tools/release_engineering/dev/push_release line 107 at r2 (raw file):

Previously, tyler-yankee wrote…

I don't have knowledge here, but strictly speaking I feel like it should remain unchanged if we want to stick as close as possible to current behavior outside of the intended change (which this is).

Sorry, my question wasn't clear. Prior to this change, the aptly repo add command used a filename without the platform. With this change, the filename now includes the platform.

@tyler-yankee
Copy link
Author

tools/release_engineering/dev/push_release line 107 at r2 (raw file):
From the docs (aptly help repo add):

When importing from directory aptly would do recursive scan looking for all files matching *.[u]deb or *.dsc
patterns. Every file discovered would be analyzed to extract metadata, package would then be created and added
to the database.

If it's searching for *.deb, then having the package name appended before .deb might muck up the search, but I'm not sure without enough experience in the use case. I'll modify it so that the old filename is used for this step just to be safe (that's what I was trying to say above, but wasn't very clear).

Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee BetsyMcPhail, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @tyler-yankee)


tools/release_engineering/dev/push_release line 107 at r2 (raw file):

Previously, tyler-yankee wrote…

From the docs (aptly help repo add):

When importing from directory aptly would do recursive scan looking for all files matching *.[u]deb or *.dsc
patterns. Every file discovered would be analyzed to extract metadata, package would then be created and added
to the database.

If it's searching for *.deb, then having the package name appended before .deb might muck up the search, but I'm not sure without enough experience in the use case. I'll modify it so that the old filename is used for this step just to be safe (that's what I was trying to say above, but wasn't very clear).

We aren't importing from a directory, though, we're giving it a file name. The changed file name reflects that the file name from AWS (and thus, the file on disk after downloading from AWS) changed. The question seems to be whether the name of the .deb on disk matters. The above-quoted documentation makes it sound like the on-disk .deb could be named baz.deb or <sha>.deb and it wouldn't matter. But, sure, if we're paranoid, maybe something like:

readonly remote_filename="drake-dev_${source_version}-1_amd64-${platform}.deb"
readonly local_filename="drake-dev_${source_version}-1_amd64.deb"

curl --fail --location -o "${local_filename} \
  "https://drake-packages.csail.mit.edu/drake/release/${remote_filename}"

@mwoehlke-kitware
Copy link
Contributor

tools/release_engineering/dev/push_release line 104 at r5 (raw file):


    curl --fail --location --remote-name \
      "https://drake-packages.csail.mit.edu/drake/release/${filename_prefix}-${platform}.deb"

This won't change the name of the file on disk... as a result, I'm fairly sure the subsequent aptly invocation will fail.

Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware 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 1 files at r6.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee BetsyMcPhail, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @tyler-yankee)


a discussion (no related file):

Previously, tyler-yankee wrote…

I also just updated the README so it contains the new filename.

:lgtm:

___ *[`tools/release_engineering/dev/push_release` line 104 at r5](https://reviewable.io/reviews/RobotLocomotion/drake/22471#-OHJjPcA366oUIj9udqg:-OHJkBXvCcNiLFI2A0zU:b-bjpj84) ([raw file](https://github.com/RobotLocomotion/drake/blob/37fe9a0fc889f978c40ebca645375a50c88e3353/tools/release_engineering/dev/push_release#L104)):*
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

This won't change the name of the file on disk... as a result, I'm fairly sure the subsequent aptly invocation will fail.

Better!

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

Successfully merging this pull request may close these issues.

3 participants