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

libxcb and xcb-proto: depend on system Python 3 #69508

Closed
wants to merge 2 commits into from

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Jan 22, 2021

The current python@3.9 formula depends on brewed tcl-tk.
For Linux, this causes an issue, as this brings in 2 new dependencies
for python@3.9: libx11 and libxext.

These both need xcb-proto and libxcb to build.
And these two dependencies already depend on python@3.9, causing
a dependency circle.

xcb-proto installs some plain python files, which are python-agnostic. Nothing is compiled here, and nothing links against xcb-proto.
The files are installed in:
/home/linuxbrew/.linuxbrew/Cellar/xcb-proto/1.13/lib/python3.9/site-packages/xcbgen/

pkg-config --variable=pythondir xcb-proto will output that path.

This path is defined by whatever Python was used to build xcb-proto.
libxcb does not build without these files.

I propose to intoduce a new Python formula which is ket only,
and which is only used to build xcb-proto and libxcb.

For most people this will not change anything as this is a build dependency only.

The formula might present some interest for users wanting a lightweight Python
version without tcl-tk.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@iMichka iMichka added the linux to homebrew-core Migration of linuxbrew-core to homebrew-core label Jan 22, 2021
@iMichka
Copy link
Member Author

iMichka commented Jan 22, 2021

Note: this is the only circular dependency I am aware of right now, so this is a quite exceptional situation.

@iMichka
Copy link
Member Author

iMichka commented Jan 22, 2021

See also:
https://trac.macports.org/ticket/26387
spack/spack#2998

There are more package managers hit by this issue: the solution is either to ship a second python like I did here, or use the system python to build xcb-proto and libxcb.

@bayandin
Copy link
Member

I propose to intoduce a new Python formula which is ket only,
and which is only used to build xcb-proto and libxcb.

For most people this will not change anything as this is a build dependency only.

Can we download compiled python (from https://www.python.org) as a resource and use it for bootstrapping? Or use a previous version of python (python@3.8)?

I don't like much an idea of adding another version of python because it could cause confusion and lead to more formula use it and then we'll need to support another version of python (and it could get even worse if it'll become versioned one).

@jonchang
Copy link
Contributor

We have had some internal formulae like meson-internal back in the day, so if we decide to have a separate formula to avoid circular xcbproto problems I think that is the direction we should go. I don't think we should encourage use of anything but the primary python@3.x formula for users.

@iMichka
Copy link
Member Author

iMichka commented Jan 22, 2021

Can we download compiled python (from https://www.python.org) as a resource and use it for bootstrapping?

I can test that, I did not think about that option.

Or use a previous version of python (python@3.8)?

I tried it and it works. The drawback is that we might migrate python@3.8 to brew tcl-tk too one day. Or when we migrate to python@3.10 we need to remove tcl-tk from python@3.9.

I don't like much an idea of adding another version of python because it could cause confusion and lead to more formula use it and then we'll need to support another version of python (and it could get even worse if it'll become versioned one).

Yeah, me neither, and regarding meson-internal: I was happy when it was gone, so it's not a solution I am looking forward to.

Let me test the bootstrapping with a pre-compiled python version, I'll keep you up-to-date once it's tested.

@iMichka
Copy link
Member Author

iMichka commented Jan 22, 2021

Regarding bootstrapping:

  • the Mac installers are .pkg installers, so not really something we can use easily
  • there is no ARM M1 compatible binary

So this does not look like a good solution.
Also, we need a relocatable python, which does involve some investment on our side, though there are already some (unexplored) solutions: https://github.com/Infinidat/relocatable-python, https://www.scylladb.com/2019/02/14/the-complex-path-for-a-simple-portable-python-interpreter-or-snakes-on-a-data-plane/.

@carlocab
Copy link
Member

we need a relocatable python

This sounds like a useful thing to have. I think, however that:

  • minimal python should not be versioned; and,
  • there should be an audit that prevents formulae from depending on minimal python unless there is a declared exception. Maybe this audit can even go further and require that dependency on minimal python must be build-only.

@fxcoudert
Copy link
Member

there should be an audit that prevents formulae from depending on minimal python unless there is a declared exception

Definitely

require that dependency on minimal python must be build-only

+100

The goal would be that end-users who don't build from source never have minimal-python installed.

@iMichka iMichka changed the title minimal-python (new formula) python-internal (new formula) Jan 23, 2021
@iMichka
Copy link
Member Author

iMichka commented Jan 23, 2021

I opened a PR in brew for the audit: Homebrew/brew#10401

Regarding the name, I went for python-internal everywhere.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

For Linux, this causes an issue, as this brings in 2 new dependencies for python@3.9: libx11 and libxext.

Just to be explicit: this is a Linux-only, build/CI only issue for us?

For most people this will not change anything as this is a build dependency only.

or use the system python to build xcb-proto and libxcb.

Given it's a need only on CI for us or folks building from source: I'd like to suggest/agree we instead add an in-formula SystemPythonRequirement that uses the Python in /usr/bin/python (or /usr/bin/python3, whatever). That means we can install (if it's not already) python in the CI environment to use just for this case.

The formula might present some interest for users wanting a lightweight Python version without tcl-tk.

This (and the audit) are reasons I'd like to avoid adding a new formulae if we can.

@iMichka
Copy link
Member Author

iMichka commented Jan 25, 2021

Just to be explicit: this is a Linux-only, build/CI only issue for us?

Yes, as tcl-tk depends on libx11 only for linux.

It might be an issue if:

  • tcl-tk starts depending on libx11 on Mac (unlikely I guess)
  • any other python@3.9 dependency starts depending on libx11 (also unlikely I guess)

Given it's a need only on CI for us or folks building from source: I'd like to suggest/agree we instead add an in-formula SystemPythonRequirement that uses the Python in /usr/bin/python (or /usr/bin/python3, whatever). That means we can install (if it's not already) python in the CI environment to use just for this case.

Let me try that. We do ship python3 in our linux CI image.

@iMichka iMichka changed the title python-internal (new formula) libxcb and xcb-proto: depend on system Python 3 Jan 25, 2021
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

This looks much better, thanks!

Formula/libxcb.rb Outdated Show resolved Hide resolved
Formula/libxcb.rb Outdated Show resolved Hide resolved
Formula/libxcb.rb Outdated Show resolved Hide resolved
Formula/xcb-proto.rb Outdated Show resolved Hide resolved
Formula/xcb-proto.rb Outdated Show resolved Hide resolved
Formula/xcb-proto.rb Outdated Show resolved Hide resolved
Formula/xcb-proto.rb Outdated Show resolved Hide resolved
Formula/xcb-proto.rb Outdated Show resolved Hide resolved
This avoids a cyclic dependency on linux.
This avoids a cyclic dependency
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

LGTM, though I do have one OCD-induced comment. But I can open another syntax-only PR if it bothers me enough 😄

Comment on lines +29 to +30
# Use Python 3, to avoid a cyclic dependency on Linux:
# python3 -> tcl-tk -> libx11 -> libxcb -> xcb-proto -> python3
Copy link
Member

Choose a reason for hiding this comment

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

You dropped the word "existing" here.

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@iMichka iMichka deleted the py39 branch January 27, 2021 12:20
@scpeters
Copy link
Member

Just to be explicit: this is a Linux-only, build/CI only issue for us?

Yes, as tcl-tk depends on libx11 only for linux.

if this is only relevant to Linux, then I think we need to add some logic to that effect, because I'm now unable to install libxcb on macOS mojave (which doesn't have python3 natively)

@scpeters
Copy link
Member

Just to be explicit: this is a Linux-only, build/CI only issue for us?

Yes, as tcl-tk depends on libx11 only for linux.

if this is only relevant to Linux, then I think we need to add some logic to that effect, because I'm now unable to install libxcb on macOS mojave (which doesn't have python3 natively)

the mojave bottle was dropped after merging this: 0232be2

@iMichka
Copy link
Member Author

iMichka commented Jan 27, 2021

Weird. Looks like there was no Mojave build? How is that.

This is a build-time dependency so you should not need it, unless you are not installing in the default prefix. But as there is no bottle ...

@carlocab
Copy link
Member

So it did:

==> Running Formulae#formula!(xcb-proto)
==> SKIPPED xcb-proto
An existing Python 3 installation is required in order to avoid cyclic
dependencies (as Homebrew's Python depends on libxcb).
==> brew fetch --retry xcb-proto --build-bottle --force
==> brew audit xcb-proto --online --git --skip-style

==> Running Formulae#formula!(libxcb)
==> SKIPPED libxcb
An existing Python 3 installation is required in order to avoid cyclic
dependencies (as Homebrew's Python depends on xcb-proto).

@iMichka
Copy link
Member Author

iMichka commented Jan 27, 2021

The fix would be:

on_macos do
  depends_on "python@3.9" => :build
end
on_linux do
  depends_on Python3Requirement => :build
end

@iMichka
Copy link
Member Author

iMichka commented Jan 27, 2021

Can someone open a pull request for libxcb and xcb-proto?

@carlocab
Copy link
Member

On it

@scpeters
Copy link
Member

I was about to volunteer, thanks @carlocab!

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Jan 27, 2021
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Jan 27, 2021
@carlocab
Copy link
Member

#69868

BrewTestBot pushed a commit that referenced this pull request Jan 27, 2021
BrewTestBot pushed a commit that referenced this pull request Jan 27, 2021
See #69508.

Closes #69868.

Signed-off-by: Michka Popoff <3406519+iMichka@users.noreply.github.com>
Signed-off-by: Steve Peters <scpeters@osrfoundation.org>
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 27, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
linux to homebrew-core Migration of linuxbrew-core to homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants