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: cd into common dir to prevent too-long filenames for windows #4630

Merged
merged 2 commits into from
Jul 17, 2022

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Feb 8, 2022

makes pyximport tests work on windows, part of #4324.

The idea is to cd into the common directory while compiling. Would it be better to do this in pyx_to_dll just before creating the ext?

@scoder scoder changed the title cd into common dir to prevent too-long filenames for windows pyximport: cd into common dir to prevent too-long filenames for windows Feb 9, 2022
@mattip
Copy link
Contributor Author

mattip commented Feb 9, 2022

Only the codestyle CI run failed, which I don't think is connected to this PR.

@scoder
Copy link
Contributor

scoder commented Feb 9, 2022

Thanks, but I wouldn't change much in pyximport right now while #4560 is coming up.

At least, this doesn't break the pyxbld test(s), which is good. (That's a well hidden and probably under-tested "feature" where pyximport allows overriding pretty much the whole build process programmatically.)

@scoder
Copy link
Contributor

scoder commented Feb 9, 2022

The code style test says /home/runner/work/cython/cython/pyximport/pyximport.py:207: [W291] trailing whitespace

@mattip
Copy link
Contributor Author

mattip commented Feb 9, 2022

Right, this will cause a conflict with #4625, so waiting is prudent. The split between pre- and post-python3.5 there will allow the newer version to use os.path.common.

@mattip
Copy link
Contributor Author

mattip commented Jul 4, 2022

The related PRs are stalled. Perhaps this could get merged first? That would move pypy/binary-testing#13 forward.

@scoder
Copy link
Contributor

scoder commented Jul 4, 2022

I was wondering if this breaks finding the .c file from the profiler, but I don't even know if profiling worked at all before when the .c file was put away into some randomly far away temporary directory. Probably not…

However, I think it would be nicer if we could shorten the long path in general, e.g. by stripping things like the user home directory and the current directory. Or by replacing the directory part or even the complete file path with a hash of that path. If pyximport controls the target directory, a hash replacement seems just fine.

@mattip
Copy link
Contributor Author

mattip commented Jul 6, 2022

If pyximport controls the target directory, a hash replacement seems just fine

I will try this

@mattip
Copy link
Contributor Author

mattip commented Jul 8, 2022

If pyximport controls the target directory

Unfortunately pyximport does not always control the build directory, it is fed into the call as a parameter. I think there is no escape from cd <workdir> in order to shorten the MSBuild compilation output.

@mattip
Copy link
Contributor Author

mattip commented Jul 8, 2022

stripping things like the user home directory and the current directory

That is the strategy I use here. I find the common directory, and cd into it. We cannot control the directory used by the build, so I think this is the best we can do.

@mattip
Copy link
Contributor Author

mattip commented Jul 9, 2022

CI is passing

@0dminnimda
Copy link
Contributor

Good, it's definitely a step further, although there are some more errors, but those seems more tame.

@mattip
Copy link
Contributor Author

mattip commented Jul 17, 2022

Any more thoughts here?

pyximport/_pyximport2.py Outdated Show resolved Hide resolved
pyximport/_pyximport3.py Outdated Show resolved Hide resolved
@mattip
Copy link
Contributor Author

mattip commented Jul 17, 2022

Thanks for the review. CI is passing.

@scoder
Copy link
Contributor

scoder commented Jul 17, 2022

I considered constraining this to Windows (since it's really a Windows-only problem), but … let's keep it general and see if it actually causes problems that way. I think having shorter path names is a virtue, in case users have to look them up.

@scoder
Copy link
Contributor

scoder commented Jul 17, 2022

Thanks @mattip

@scoder scoder merged commit 344389e into cython:master Jul 17, 2022
@stefanor
Copy link
Contributor

@mattip: Found some breakage in this, see #5957.

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

Successfully merging this pull request may close these issues.

None yet

4 participants