[workspace] Avoid crashes in python repository rule

[jwnimmer-tri]

(Refer to the commit message for details.)

Towards #20556 and #14967.

---

This change is 

[lepalom]

I have cherry picked the changes in a debian bookworm, but I still have a crash. Changes are here. Take care because there are more changes related to build it in Debian. Please. @jwnimmer-tri test it.

[jwnimmer-tri]

Here's some results from my experimentation: https://github.com/jwnimmer-tri/drake/commits/bookworm.

The first commit just adds some Dockerfile stuff to make it easier for me to test (along with a list of Build-Depends, in effect).
The next two commits implement the "Presume unknown systems works like Ubuntu Jammy" trick I mentioned before.
The last commit fixes a hard-coded path to libclang.
With all of that together, the make install step passes.

Longer term, trying to add "Debian" as a distinct OS flavor from "Ubuntu" like you were trying to do in your branch will not scale. Instead, we need to refactor Drake to ask "is_linux" instead of "is_ubuntu".

The Python change here is probably useful once we have "is_linux" more generally, but I'm not sure it's of much help just on its own.

For now though, those patches should let you build from source and keep exploring from there.

[jwnimmer-tri]

@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please

@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]

+@mwoehlke-kitware for feature review, please.

[mwoehlke-kitware]

Dropping the supported-version check is okay. The changes to how we specify test programs on the command line is fine. The other refactoring seems unnecessary and unhelpful?

Reviewable status: 2 unresolved discussions, LGTM missing from assignee mwoehlke-kitware, needs platform reviewer assigned, needs at least two assigned reviewers

---

-- commits line 7 at r2:
"in the a rule"? I think the "a" doesn't belong? (Is Reviewable even going to preserve this information on merge? I can never remember which of the commit(s) or description gets used under what circumstances.)

---

tools/workspace/python/repository.bzl line 46 at r2 (raw file):

    """

    # - `is_any_macos`

Why "is_any_macos"? Why not "is_macos" like os_result?

Note: if the reason is to set this if osr.is_macos or osr.is_macos_wheel, it would be clearer to retain determine_os and say thusly. It would also avoid duplicating the homebrew_prefix logic. (Why isn't determine_os being used, anyway?)

[mwoehlke-kitware]

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, needs platform reviewer assigned, needs at least two assigned reviewers

[jwnimmer-tri]

The other refactoring seems unnecessary and unhelpful?

I'm happy to work on these but I'm not sure exactly what you mean. Was it only the is_any_macos discussion, or more than that?

Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, needs platform reviewer assigned, needs at least two assigned reviewers

---

-- commits line 7 at r2:

"in the a rule"? I think the "a" doesn't belong?

Yup, fixed.

(Is Reviewable even going to preserve this information on merge? I can never remember which of the commit(s) or description gets used under what circumstances.)

The commit message(s) are always intact (and the PR description is always ignored), unless the platform reviewer edits the text in the merge button drop-down box as they are merging. (Typically they will only do that when the PR is cluttered with uncurated incremental pushes of "fix", "typo" etc.)

---

tools/workspace/python/repository.bzl line 46 at r2 (raw file):

Why isn't determine_os being used, anyway?

The overall goal of the PR train is to delete determine_os entirely. There's only a couple more places to purge after this one.

Thus, the homebrew_prefix duplication is only temporary. (The only other use of os_result.homebrew_prefix is libjpeg/repository.bzl which is deprecated and will be deleted later this week.)

Why not "is_macos" like os_result?

Your intuition was right -- in os_result, the variable is_macos meant "Building a full release from source, using Homebrew", so in wheel builds on OS X is_macos would be false. Since this variable had different semantics, I was using a different name for it.

While considering how to choose a better name, I took a more careful look and found a bunch of other things, now pushed as changes:

• There was really no reason to return anything about macOS in the helper struct, so this now is just deleted entirely.

• Purging -lpython3.# out of the module linkopts seems like it was vestigial from the days of Ubuntu 16.04 or earlier, so that's deleted now too. (No python3.#-config I tested on any platform seems to produce that flag anymore.)

• The explanation for the two different link flavors was awkward. I rewrote the docs (and variable names) to hopefully better explain what we're trying to do, and how we're doing it.

PTAL.

[jwnimmer-tri]

@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please

@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

[mwoehlke-kitware]

Okay, I think it , though the nitty-gritty of Python linking is somewhat outside my area of expertise and might not hurt to have another set of eyes. (Of course, we still have platform review...)

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

---

tools/workspace/python/repository.bzl line 46 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why isn't determine_os being used, anyway?

The overall goal of the PR train is to delete determine_os entirely. There's only a couple more places to purge after this one.

Thus, the homebrew_prefix duplication is only temporary. (The only other use of os_result.homebrew_prefix is libjpeg/repository.bzl which is deprecated and will be deleted later this week.)

Why not "is_macos" like os_result?

Your intuition was right -- in os_result, the variable is_macos meant "Building a full release from source, using Homebrew", so in wheel builds on OS X is_macos would be false. Since this variable had different semantics, I was using a different name for it.

While considering how to choose a better name, I took a more careful look and found a bunch of other things, now pushed as changes:

• There was really no reason to return anything about macOS in the helper struct, so this now is just deleted entirely.

• Purging -lpython3.# out of the module linkopts seems like it was vestigial from the days of Ubuntu 16.04 or earlier, so that's deleted now too. (No python3.#-config I tested on any platform seems to produce that flag anymore.)

• The explanation for the two different link flavors was awkward. I rewrote the docs (and variable names) to hopefully better explain what we're trying to do, and how we're doing it.

PTAL.

Okay; that's rather important information to know... I will be interested to see what happens to the (by my count) other eight uses...

BTW, the 'determine homebrew prefix' logic also exists in tools/workspace/execute.bzl.

[jwnimmer-tri]

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

---

tools/workspace/python/repository.bzl line 46 at r2 (raw file):

Okay; that's rather important information to know...

Good point! Disclaimer added.

I will be interested to see what happens to the (by my count) other eight uses...

Several of them are in deprecated rules that will be deleted later this week.

BTW, the 'determine homebrew prefix' logic also exists in tools/workspace/execute.bzl.

Ah, I missed that in my searching (because it was the bin path instead of the whole prefix). I've refactored now to reduce duplication.

[jwnimmer-tri]

The nitty-gritty of Python linking is somewhat outside my area of expertise and might not hurt to have another set of eyes.

Luckily @rpoyner-tri knows a bit about this. I'll also \CC @EricCousineau-TRI for awareness, and/or in case you'd like to take a look.

[jwnimmer-tri]

@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please

@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

[mwoehlke-kitware]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

---

tools/workspace/execute.bzl line 9 at r5 (raw file):

    # paths.
    if repo_ctx.os.name == "mac os x":
        arch_result = repo_ctx.execute(["/usr/bin/arch"])

FYI, we're confident /usr/bin/arch and repo_ctx.os.arch are the same? (I guess CI is likely to fail otherwise?)

---

tools/workspace/python/repository.bzl line 46 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Okay; that's rather important information to know...

Good point! Disclaimer added.

I will be interested to see what happens to the (by my count) other eight uses...

Several of them are in deprecated rules that will be deleted later this week.

BTW, the 'determine homebrew prefix' logic also exists in tools/workspace/execute.bzl.

Ah, I missed that in my searching (because it was the bin path instead of the whole prefix). I've refactored now to reduce duplication.

I meant in terms of "why is the review making this change", but okay, the disclaimer won't hurt. 🙂

Thanks for refactoring homebrew_prefix.

[jwnimmer-tri]

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

---

tools/workspace/execute.bzl line 9 at r5 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

FYI, we're confident /usr/bin/arch and repo_ctx.os.arch are the same? (I guess CI is likely to fail otherwise?)

Yup. The string names they use to notate the arches differ, but whether they return the intel name vs the aarch name is consistent.

[mwoehlke-kitware]

tools/workspace/execute.bzl line 9 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yup. The string names they use to notate the arches differ, but whether they return the intel name vs the aarch name is consistent.

👍 (And, sure, string names aren't important.)

FYI, I subsequently remembered where it is that we play games with arch and reexecing into a different environment. I thought I recalled that wheel builds did something funky, which was why I was concerned, but it's CI (Jenkins always launches jobs in x86 mode; n.b. logic in ctest_driver_script_wrapper.bash). That gets sorted early enough that I wouldn't expect problems by the time Bazel is running. Now that I remember what specifically was making me suspicious, I am more confident it isn't an issue here.

[mwoehlke-kitware]

I think this is ready to be kicked over to @rpoyner-tri for platform review?

[jwnimmer-tri]

Oops, I thought I had. +@rpoyner-tri for platform review per schedule, please.

[rpoyner-tri]

Reviewed 1 of 2 files at r2, 3 of 3 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion

---

tools/workspace/python/repository.bzl line 140 at r5 (raw file):

    linkopts = [linkopt for linkopt in linkopts if linkopt]

    # Undo whitepsace splits for options with a positional argument, e.g., we

typo

Suggestion:

whitespace

[rpoyner-tri]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),mwoehlke-kitware