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

Figure out how to support dynamic data_files #127

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vidartf
Copy link
Collaborator

@vidartf vidartf commented Feb 21, 2022

Document how to recursively include data files.

Also leaves open a question about how to perform such recursive inclusion of data files dynamically.

Document how to recursively include data files.

Also leaves open a question about how to perform such recursive inclusion of data files dynamically.
def build_with_datafiles():
builder()
# needed as some dirs are generated as part of the build (or lazy):
<need reference to dist here!>.data_files = get_data_files([
Copy link
Collaborator Author

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?

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 called after the build step, which means the glob should work on files that are there after building.

Copy link
Collaborator Author

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" ?

Copy link
Contributor

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 the data directory in the wheel, see #128

Copy link
Member

@minrk minrk Mar 25, 2022

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:

  1. run builder() unconditionally before get_data_files() before passing it to setup
  2. run builder() in the normal fashion in pre_dist and then modify dist.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.

Copy link
Member

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.

  • if generated files are not present: always run the build and skip the custom command classes (e.g. install from git or git-archive)
  • If they are present
    • and running from git: hook up command classes, since they may want rebuilds
    • and not running from git (probably sdist): skip rebuild

Copy link
Contributor

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.

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.

3 participants