-
Notifications
You must be signed in to change notification settings - Fork 77
ENH: add support for symbolic links in package repository #728
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
# SPDX-FileCopyrightText: 2025 The meson-python developers | ||
# | ||
# SPDX-License-Identifier: MIT |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
/__init__.py |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# SPDX-FileCopyrightText: 2025 The meson-python developers | ||
# | ||
# SPDX-License-Identifier: MIT | ||
|
||
project('symlinks', version: '1.0.0') | ||
|
||
py = import('python').find_installation() | ||
|
||
py.install_sources( | ||
'__init__.py', | ||
'submodule/__init__.py', | ||
'submodule/aaa.py', | ||
'submodule/bbb.py', | ||
subdir: 'symlinks', | ||
preserve_path: true, | ||
) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# SPDX-FileCopyrightText: 2021 The meson-python developers | ||
# | ||
# SPDX-License-Identifier: MIT | ||
|
||
[build-system] | ||
build-backend = 'mesonpy' | ||
requires = ['meson-python'] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../__init__.py |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../__init__.py |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
# SPDX-FileCopyrightText: 2025 The meson-python developers | ||
# | ||
# SPDX-License-Identifier: MIT | ||
|
||
def foo(): | ||
return 42 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
aaa.py |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR. I am curious - what would be the suggested approach to disable this warning?
In the original PR I saw nanoarrow discussed, so I just wanted to clarify the use case there. With nanoarrow being a multi-language project, the repo is laid out like:
So the point of the symlink in python/subprojects is to allow the python module to be built on the sources in the
c
directory. When the sdist is created, we are happy to not have any symlinked content included, as we expect the user to have the c sources installed on the system.So in our case the warnings are unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW that comment applies to really allow of the Arrow libraries (pyarrow, nanoarrow, adbc-driver-<driver_name>) as they are all multi-language projects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning is just a warning that tells you that your symbolic links will not be included in the sdist, as the sdist format does not allow them. Would you prefer meson-python to drop the symlinks silently?
Anyhow, That source layout, unless there is a
pyproject.toml
in the root directory, like thiswould result in an invalid Python sdist when created with meson-python. Thus the fact that the warning is emitted in your case is completely irrelevant: you cannot use the result of the operation emitting a warning for anything useful, thus maybe do not perform the operation at all.
I think this way of organizing the project is problematic: it bundles the sources for the C and Python parts in the same repository, thus coupling them tightly, but at the same time wants to treat the Python part as a separate project which is loosely coupled to the C part at build time. I don't know how you came to choose this organization, but I have the impression that it creates way more problems that it solves.
What I would have done is to have the C and Python parts in two separate repositories. The Python part would bring in the C part either as a git submodule or as a Meson subproject or as a Meson subproject referenced as a git submodule.
But it is your project, so you can organize it as you like, and if you like mono-repos, you can have one. However, you are choosing to work against established best practices, thus do not expect tools to bend their workings to accommodate your choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that the case, their repository layout cannot be packaged into a Python sdist as symbolic links are not supported by the sdist standard. Anyway, their repository structure does not work for creating a sdist with meson-python. Thus I don't see the problem: these project choose to make things complicated for themselves and will need to find an ad-hoc way to generate a valid sdist from their monorepo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also possible to only publish wheels, and not sdists, which is no doubt a tempting proposition to much scientific code that has painful sdist-building workflows (esp. doesn't work well at all on Windows)
... but in this case that's exactly what their plan is, right? Not to use it, but only have it exist as a convenience when already building inside the monorepo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of nanoarrow, what we do is use a symlink for local development (so that a user can test C/python changes all at once), but for a sdist we end up using a custom dist script that replaces the symlink with a copy of the files needed to build a standalone Python library.
https://github.com/apache/arrow-nanoarrow/blob/4c1e484ca7d575250444e0a8eee5884e13489104/python/generate_dist.py
While the majority of users I would think are fine with pre-packaged wheels, I would be hesitant to rely on that exclusively. Using PyArrow as an example, there are legitimate use cases where end users do not want all of the standard functionality the package bundles with its C/C++ extensions, so they end up building from source with their own build options. I know that the version of PyArrow provided in lambda containers turns off some build features that the standard wheel provides, because AWS Lambda has certain size restrictions and the default build is too large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A dist script for sdist generation sounds quite reasonable.
Re the repo layout: this is actually pretty common especially for older C and C++ projects with optional Python bindings. It'd be good to support that use case explicitly via a test package; we've encountered it multiple times by now (not suggesting it needs to be part of this PR).
Thinking about it more: I like the
nanoarrow
repo setup I think. It's probably easier to deal with a C++ library in..
as a subproject than it would be to use the top-levelmeson.build
file as the entry point for C++ and Python packages at once.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this PR with
nanoarrow
'smain
branch - sdist and wheel builds work as expected, and no warnings are visible. So I'll go ahead with merging this PR. We can open a new issue to keep track of the use case and adding a test package and/or some docs (I'm interested in doing that, but no time for it right now).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in gh-744
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rgommers !