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

pyximport: Use relative paths less, and correctly #5957

Merged
merged 2 commits into from Feb 1, 2024

Conversation

stefanor
Copy link
Contributor

@stefanor stefanor commented Jan 21, 2024

#4630 started to use relative paths in pyximport, when the path got long, to avoid issues on Windows.

This has two issues:

  1. A bug, the path calculation assumed that the common path would be the same as the CWD.
  2. It loses information, on platforms that don't have this filesystem limitation. In particular, it breaks pretty traceback formatting in stack_data, if the source file isn't available at the same relative path.

Discoverd these while investigating Debian bug #1056872

# Windows concatenates the pyxbuild_dir to the pyxfilename when
# compiling, and then complains that the filename is too long
common = os.path.commonprefix([pyxbuild_dir, pyxfilename])
if len(common) > 30:
pyxfilename = os.path.relpath(pyxfilename)
pyxfilename = os.path.relpath(pyxfilename, common)
pyxbuild_dir = os.path.relpath(pyxbuild_dir)
os.chdir(common)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bug fix, since the chdir should have preceded the shortening of pyxfilename and pyxbuild_dir:

os.chdir(common)
pyxfilename = os.path.relpath(pyxfilename)
pyxbuild_dir = os.path.relpath(pyxbuild_dir)

Shouldn't the fix in the PR also apply to pyxbuild_dir?:

pyxbuild_dir = os.path.relpath(pyxbuild_dir, common)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chdir should have preceded the shortening of pyxfilename and pyxbuild_dir

Is there a guarantee that pyxfilename is absolute? If not, if you chdir first, your path to pyxfilename won't be correct any more.

Shouldn't the fix in the PR also apply to pyxbuild_dir?:

Oh, yeah, probably

It should be relative to the new current directory, not the old one.
Using a relative path means we lose path information in the built
module. This can be useful in packages such as stack_data that look up
source code.
Copy link
Contributor

@mattip mattip left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. I did try out PR #4630, but maybe there was something in the test directory structure that make relpath work. This fix is better.

@stefanor
Copy link
Contributor Author

Hrm, the downside of the second commit is that baking in absolute paths is bad for non-reproducibility (Debian #988999). However, that should be solved systematically, not with a 30-character heuristic.

@mattip
Copy link
Contributor

mattip commented Jan 21, 2024

I am confused. How does os.path.relpath end up turning a relative path into an absolute one?

@stefanor
Copy link
Contributor Author

What I meant is that I'm perpetuating the non-reproducibility problem by not using relpath on non-Windows.

@da-woods da-woods added this to the 3.0.9 milestone Feb 1, 2024
@da-woods da-woods merged commit 09863a7 into cython:master Feb 1, 2024
63 checks passed
da-woods pushed a commit that referenced this pull request Feb 1, 2024
* pyximport: Calculate the relative path correctly

It should be relative to the new current directory, not the old one.

* pyximport: Avoid chdir before build on non-Windows

Using a relative path means we lose path information in the built
module. This can be useful in packages such as stack_data that look up
source code.
@da-woods
Copy link
Contributor

da-woods commented Feb 1, 2024

Thanks - 3.0.x backport in 84273d6

@stefanor stefanor deleted the pyximport-common-path branch February 4, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants