implement the depfile command-line option for the "cython" tool #4916
Conversation
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 looks pretty clean to me so far; the one code block moved into |
6c8137d to
defbddb
Compare
|
Tests pass locally. |
scoder
reviewed
Jul 27, 2022
Cython/Build/Dependencies.py
Outdated
Comment on lines
724
to
742
| def write_depfile_for(self, source, c_file): | ||
| dependencies = self.all_dependencies(source) | ||
| src_base_dir, _ = os.path.split(source) | ||
| cwd = os.getcwd() | ||
| if not src_base_dir.endswith(os.sep): | ||
| src_base_dir += os.sep | ||
| # paths below the base_dir are relative, otherwise absolute | ||
| paths = [] | ||
| for fname in dependencies: | ||
| if fname.startswith(src_base_dir): | ||
| paths.append(os.path.relpath(fname, cwd)) | ||
| else: | ||
| paths.append(os.path.abspath(fname)) | ||
|
|
||
| depline = os.path.relpath(c_file, cwd) + ": \\\n " | ||
| depline += " \\\n ".join(paths) + "\n" | ||
|
|
||
| with open(c_file+'.dep', 'w') as outfile: | ||
| outfile.write(depline) |
There was a problem hiding this comment.
Excluding the call to self.all_dependencies(), I think this can be moved to Utils.py where most of the file related utilities live. It's generally a good idea to separate calculation (i.e. figuring out the dependencies) from representation (i.e. writing them down into a dependency file).
defbddb to
5de9e30
Compare
scoder
reviewed
Jul 28, 2022
We would like to use it in the frontend for the cython tool as well. Pull it out of the main loop and make it a utility function.
The paths for files in the source tree are relative instead of absolute. This is not inherently a problem, but they are relative to the containing directory of the source code file but the program that parses these files (make, ninja) computes filenames relative to its working directory, not relative to the source file. Likewise, the final output file had its entire path component trimmed, leaving just a bare filename. Make this actually work by computing all relative paths relative to the current working directory of the cython process itself. When invoked by a build system, this will be the same directory the build system expects files to be based on.
This acts the same as the option for the cythonize tool.
4046697 to
c631e7d
Compare
|
Thanks! Is there interest in backporting this to 0.29.x? |
|
Backporting to 0.29.x would need some minor manual tweaks: #4576 |
|
Alright, I tried a backport. |
scoder
pushed a commit
that referenced
this pull request
Aug 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Tested with the cythoner from SciPy after editing in the -M argument and updating lots of meson generators to expect a depfile.
/cc @rgommers @ev-br
I do not like the delayed import of
Build.Dependencies.DependencyTreebut it is needed to avoid a circular import, if anyone has suggestions to fix this I'm all ears. (I think this would require more refactoring than I'm comfortable with, but TBH the code is massively confusing in layout to me.)This is missing tests but I have no time left today for that (I'll finish this up on Sunday), so, posting for discussion.