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

Enhancement to devguide for MacOS (and 3.11?) #976

Open
smontanaro opened this issue Oct 31, 2022 · 3 comments
Open

Enhancement to devguide for MacOS (and 3.11?) #976

smontanaro opened this issue Oct 31, 2022 · 3 comments

Comments

@smontanaro
Copy link
Contributor

smontanaro commented Oct 31, 2022

This bug report is related to my recent experience building from source on my shiny new MacBook Pro, and my crash-and-burn experiences creating and managing a PR against the argparse documentation (and further discussion of the mess I made in another PR.

In my attempts to create and manage a pull request, I screwed up some stuff and discovered some other stuff distinct to MacOS. I will just identify them at this point and worry about creating a PR later (not sure I even have the devguide repo checked out at this point).

  • In the pull request steps the user is instructed to create a branch against upstream/main. For naive Git users such as myself, it fails to mention that I need to explicitly set that branch to track origin/<branch>. This created some confusion on my part.
  • Maybe this is a non-issue, but since the user is expectly to have forked python/cpython it's probably worth making it very explicit that the branch for the PR must be created against upstream/main, not origin/main. The git command mentions upstream, but I think it might be worth a "Note: do not branch locally or against your fork." Again, I have never been (and never will be) an expert Git user, so the subtle details about why this is so will probably always be lost on me. Better to be explicit than implicit...
  • When installing Homebrew packages (some, though perhaps not all) will not have the necessary pkgconfig symlinks created automatically. Homebrew indicates this (in an I think indirect manner) saying something to the effect that such packages aren't exposed (not their verb) because there are Apple versions already available. I think this means that pkgconfig symlinks aren't created. In my case (MacOS 13/Ventura), this was true for the openssl and tcl-tk packages. xz was already installed by default. Before I realized this was the case, I installed openssl@3 and created its symlinks. In retrospect, [email protected] was installed by default. All I needed to do was create pkgconfig symlinks. I had to install gdbm, but there aren't any pkgconfig symlinks, so I will eventually have to figure out command line args for its discovery. Adding pkgconfig symlinks was a lot easier than handcrafting the relevant -L, -I, and pkgconfig path magic.
  • Finally, after creating those symlinks, I didn't need to do any CFLAGS, LDFLAGS or PKG_CONFIG_PATH dance (well, except for gdbm). Now I am down to this configure command: ./configure GDBM_CFLAGS=-I/opt/homebrew/include GDBM_LIBS=-L/opt/homebrew/lib. I don't know if this applies to 3.11 or just the nascent 3.12, but I suspect a third section needs to be added to reflect the fact that it's clearly getting better. I don't know if that's the Python devs', Apple's, or Homebrew's doing, but kudos to whoever did this.

Edit: make the GDBM_LIBS -L/opt/homebrew/lib -lgdbm :doh:

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Oct 31, 2022

The exact command for macOS mentioned in the devguide works fine for me (I have working tkinter, ssl, etc):

CFLAGS="-I$(brew --prefix gdbm)/include -I$(brew --prefix xz)/include" \
  LDFLAGS="-L$(brew --prefix gdbm)/lib -I$(brew --prefix xz)/lib" \
  PKG_CONFIG_PATH="$(brew --prefix tcl-tk)/lib/pkgconfig" \
  ./configure --with-pydebug \
              --with-openssl=$(brew --prefix openssl)

Just to clarify, does this^ not work for you, or is it more that it's clunky — or both?

@smontanaro
Copy link
Contributor Author

smontanaro commented Oct 31, 2022

Sorry, I never checked whether or not it worked, though I have no reason to doubt that it does. It would just have taken me several (many?) tries to converge on that solution. I know that the simpler configure command I gave:

./configure GDBM_CFLAGS=-I/opt/homebrew/include GDBM_LIBS=-L/opt/homebrew/lib

Does work. I don't know if something changed with configure itself, but I suspect it might have. It would seem it attempts to use pkg-config to determine compile/link flags as much as possible. It can't for GDBM because that package simply fails to provide them.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 1, 2022

In the pull request steps the user is instructed to create a branch against upstream/main. For naive Git users such as myself, it fails to mention that I need to explicitly set that branch to track origin/. This created some confusion on my part.

It seems the Step by step PR lifecycle guide doesn't set an upstream tracking branch on first push (i.e. with the -u flag), as is standard in most workflows, and Git's own output recommends? This means all the other commands specified there manually specify the remote and branch name every time, which will work as stated, but adds complexity for users and increases the chance of them later pushing or pulling from the wrong branch, as happened here, is there a reason for not just setting an upstream tracking branch on first push (i.e. passing -u) to simplify all this?

Maybe this is a non-issue, but since the user is expectly to have forked python/cpython it's probably worth making it very explicit that the branch for the PR must be created against upstream/main, not origin/main. The git command mentions upstream, but I think it might be worth a "Note: do not branch locally or against your fork." Again, I have never been (and never will be) an expert Git user, so the subtle details about why this is so will probably always be lost on me. Better to be explicit than implicit...

To be clear, this didn't cause any of your issues; the main thing is you want to be working on as up to date a copy of the branch as possible to avoid merge conflicts and touching out of date stuff. So long as the user updates/has recently updated their local main branch before starting their new feature branch from it, they don't have to actually use that command, but doing so will ensure their branch is up to date without having to do the extra step of switching to main and then pulling upstream before switch -cing to a new branch. So long as users actually follow the instructions, they will be fine, but otherwise the worst that might happen is they have to resolve a merge conflict (which may happen regardless, as there are likely to be many commits to main between when they start work and when their PR is ready to be merged.

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

No branches or pull requests

3 participants