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

[workspace] Simplify several externals to be platform-agnostic #20606

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Nov 29, 2023

Towards #14967. We only want to lock ourselves to specific Ubuntu release numbers in cases where we acutely care. In most cases, we only care about "linux" vs "darwin", not Ubuntu.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits release notes: none This pull request should not be mentioned in the release notes labels Nov 29, 2023
@jwnimmer-tri jwnimmer-tri changed the title [workspace] Simplify several externals to be more platform-agnostic [workspace] Simplify several externals to be platform-agnostic Nov 29, 2023
@jwnimmer-tri jwnimmer-tri force-pushed the os-portability-4linux branch 2 times, most recently from 897d798 to df6b0ba Compare November 29, 2023 20:36
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review November 29, 2023 20:45
@jwnimmer-tri jwnimmer-tri added the status: single reviewer ok https://drake.mit.edu/reviewable.html label Nov 29, 2023
@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-release please
@drake-jenkins-bot mac-arm-ventura-unprovisioned-clang-wheel-experimental-release please
@drake-jenkins-bot mac-x86-monterey-unprovisioned-clang-wheel-experimental-release please

@jwnimmer-tri
Copy link
Collaborator Author

+@EricCousineau-TRI for both(?) reviews, please. If you'd like a separate feature reviewer, we could add Rico for that.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-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_strong: x2

Reviewed 1 of 1 files at r1, 2 of 2 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)

Copy link
Contributor

@EricCousineau-TRI EricCousineau-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, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)


tools/workspace/mosek/repository.bzl line 33 at r5 (raw file):

    mosek_patch_version = 46

    os_name = repository_ctx.os.name  # 'linux' or 'mac os x'

nit (minor) This file does no mapping,but gurobi mapped from mac os x to darwin.

Is it useful for other if this were consistent?

Copy link
Collaborator Author

@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: :shipit: complete! all discussions resolved, LGTM from assignee EricCousineau-TRI(platform), base commit is latest master


tools/workspace/mosek/repository.bzl line 33 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit (minor) This file does no mapping,but gurobi mapped from mac os x to darwin.

Is it useful for other if this were consistent?

I went back and forth on different spellings in the various files (both here, and in other related PRs). Your comment was enough to tip this one the other way.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-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 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignee EricCousineau-TRI(platform), base commit is latest master


tools/workspace/mosek/repository.bzl line 33 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I went back and forth on different spellings in the various files (both here, and in other related PRs). Your comment was enough to tip this one the other way.

Aye, thanks!

@jwnimmer-tri jwnimmer-tri merged commit fa2f4a3 into RobotLocomotion:master Nov 30, 2023
@jwnimmer-tri jwnimmer-tri deleted the os-portability-4linux branch November 30, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: none This pull request should not be mentioned in the release notes status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants