-
Notifications
You must be signed in to change notification settings - Fork 47
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
Figure out how to support dynamic data_files #127
Draft
vidartf
wants to merge
1
commit into
main
Choose a base branch
from
dynamic-recursive-datafiles
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
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 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
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.
This is the issue. How do we dynamically update the data files here? Do we need to pass a reference of the
Command
to the builder function?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.
This is called after the build step, which means the glob should work on files that are there after building.
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'm confused. To be clear, I expect
get_data_files
to work here, but the question is what we do with that list of files? What attribute do we set to tell the build system "hey! add these files to the collection of data files" ?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 gets passed to
setuptools.setup()
. I much prefer the way flit handles data files now: you give it a populated directory and it maps it right to thedata
directory in the wheel, see #128There 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 just ran into this in IPython Parallel. The problem here is that the data files aren't known until after
builder()
is called, and can change if builder is called to regenerate files already present (see ipython/ipyparallel#675). So the choice is:builder()
unconditionally beforeget_data_files()
before passing it to setupbuilder()
in the normal fashion inpre_dist
and then modifydist.data_files
. This is why the deprecated implementations had the handle_files. This step is still required to produce correct output, but it has been removed.It can be helped if these hooks were passed the distribution as an arg, rather than no args.
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.
Perhaps the simplest (and most flit-like) way to do it is instead of running the builder as a pre_dist, etc. step, run it at the top-level of setup.py (perhaps conditional on some sys.argv to avoid triggering it too often).
That's what I ended up doing in ipython/ipyparallel#680 with extra steps to avoid the unnecessary rebuilds that caused ipython/ipyparallel#675.
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 @minrk, I did something similar in jupyterlab/jupyterlab#12208. I agree we should make this cleaner and provide better guidance.