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

Make setup_py tasks provide 'python_dists' product. #4498

Merged
merged 3 commits into from May 9, 2017

Conversation

Projects
None yet
4 participants
@thesamet
Copy link
Contributor

thesamet commented Apr 20, 2017

This enables subsequent tasks to consume the artifacts produced by the setup_py task.

Problem

See #4478

Solution

Add python_dists key to ProductMapping that contains either the path to tar.gz in case no run-options we provided, otherwise a path to the source distribution where than run command got executed.

@thesamet

This comment has been minimized.

Copy link
Contributor

thesamet commented Apr 20, 2017

I changed the execute() behavior to return the ProductMapping rather than a dictionary and adjusted the tests accordingly. The thinking was that maybe in the future the tests can be written such that they directly consume this product, rather than a different structure.

@@ -1 +1 @@
06e4c8ac8a3cf4f20cb74929208f35ab4fe75d0b

This comment has been minimized.

@jsirois

jsirois Apr 21, 2017

Member

This change is causing 3 CI shards to fail. You might revert the change to this file and/or rebase against master, then confirm by running pants, something like:

rm -rf ~/.cache/pants/bin/native-engine/
./pants

That should re-build the native engine locally and it should cause no change to the contents of src/python/pants/engine/subsystem/native_engine_version.

This comment has been minimized.

@thesamet

thesamet Apr 22, 2017

Contributor

Not sure how I ended up staging this file. Will amend the commit.

@thesamet thesamet force-pushed the thesamet:setup-py-prods branch 2 times, most recently from 6029e5d to f189669 Apr 22, 2017

@thesamet thesamet force-pushed the thesamet:setup-py-prods branch from 870a750 to f40904f Apr 29, 2017

@stuhood stuhood requested a review from benjyw May 6, 2017

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 6, 2017

Sorry for the delay here @thesamet ... @benjyw : do you have time to look at this one?

There should be enough time to get this in 1.3.0. Will be cutting the first rc on Sunday most likely.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented May 8, 2017

Sorry, was AFK all weekend, canyoneering and hiking in Zion. Looking now.

@benjyw
Copy link
Contributor

benjyw left a comment

Thanks for this! Looks useful. See my comments inline.

executed[target] = setup_dir
return executed
python_dists.add(target, os.path.dirname(setup_dir)).append(os.path.basename(setup_dir))
return python_dists # returned for tests

This comment has been minimized.

@benjyw

benjyw May 8, 2017

Contributor

Not your fault, but this was always yucky. Can you add a TODO to modify the tests to get this data from the products, and remove this return?

This comment has been minimized.

@thesamet

thesamet May 9, 2017

Contributor

Actually did the TODO, it was fairly easy.

This comment has been minimized.

@benjyw

benjyw May 9, 2017

Contributor

You are my hero.

@@ -298,6 +298,10 @@ def is_resources_target(target):
def has_provides(cls, target):
return cls.is_python_target(target) and target.provides

@classmethod

This comment has been minimized.

@benjyw

benjyw May 8, 2017

Contributor

Is it necessary to modify the old task? I would rather not make changes to about-to-be-deprecated code, as it adds maintenance burden. Can we make availability of this product contingent on switching to the new pipeline?

This comment has been minimized.

@thesamet

thesamet May 9, 2017

Contributor

Reverted.

interpreter = self.context.products.get_data(PythonInterpreter)
python_dists = self.context.products.get('python_dists')

This comment has been minimized.

@benjyw

benjyw May 8, 2017

Contributor

We prefer use of get_data (and the other _data method variant) which allows you to set the product to whatever data structure you like, instead of the older get, which forces use of ProductMapping, and requires futzing around with target base dirs and other annoyances.

See the docstring on pants.goal.products.Products for details.

This comment has been minimized.

@thesamet

thesamet May 9, 2017

Contributor

Nice, wasn't aware of this. Much cleaner now.

@@ -607,10 +611,10 @@ def create(target):
self.context.log.info('Writing {}'.format(sdist_path))
shutil.move(setup_runner.sdist(), sdist_path)
safe_rmtree(setup_dir)
executed[target] = sdist_path
python_dists.add(target, os.path.dirname(sdist_path)).append(os.path.basename(sdist_path))

This comment has been minimized.

@benjyw

benjyw May 8, 2017

Contributor

The 'basedir' is generally expected to be a "package root" or something similar. There's no such concept here, so you're effectively having to use the file's immediately enclosing dir as a fake basedir. This is why it's better to use "data products", where you can pick your own data structure, and not worry about this. See above.

This comment has been minimized.

@thesamet

thesamet May 9, 2017

Contributor

Done

@thesamet thesamet force-pushed the thesamet:setup-py-prods branch 2 times, most recently from f2ab942 to 7be2df7 May 9, 2017

thesamet added some commits Apr 20, 2017

Make setup_py tasks provide 'python_dists' product.
This enables subsequent tasks to consume the artifacts produced
by the setup_py task.

@thesamet thesamet force-pushed the thesamet:setup-py-prods branch from 7be2df7 to 7f9c30d May 9, 2017

@thesamet

This comment has been minimized.

Copy link
Contributor

thesamet commented May 9, 2017

Done, PTAL.

@stuhood stuhood added this to the 1.3.0 milestone May 9, 2017

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 9, 2017

Thanks! Targeted this at 1.3.0. cc @benjyw

@benjyw

benjyw approved these changes May 9, 2017

Copy link
Contributor

benjyw left a comment

Thanks! LGTM modulo the product constant name change.

@@ -278,6 +278,8 @@ class SetupPy(Task):

SOURCE_ROOT = b'src'

PRODUCT_TYPE = 'python_dists'

This comment has been minimized.

@benjyw

benjyw May 9, 2017

Contributor

Better to call this something like PYTHON_DISTS_PRODUCT, in case this task is ever modified to produce more than one product in the future.

executed[target] = setup_dir
return executed
python_dists.add(target, os.path.dirname(setup_dir)).append(os.path.basename(setup_dir))
return python_dists # returned for tests

This comment has been minimized.

@benjyw

benjyw May 9, 2017

Contributor

You are my hero.

@thesamet

This comment has been minimized.

Copy link
Contributor

thesamet commented May 9, 2017

Done. Thanks for the quick review.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 9, 2017

One flaky test ... re-running, and will merge on green. Thanks @thesamet .

@stuhood stuhood merged commit f24ceb8 into pantsbuild:master May 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

stuhood added a commit that referenced this pull request May 12, 2017

Make setup_py tasks provide 'python_dists' product. (#4498)
### Problem

See #4478 

### Solution

Add `python_dists` key to ProductMapping that contains either the path to tar.gz in case no run-options we provided, otherwise a path to the source distribution where than run command got executed.

### Result

This enables subsequent tasks to consume the artifacts produced by the setup_py task.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment