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

allow all string (not binary!) types for ExecuteProcessRequest args #7529

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

commented Apr 10, 2019

Problem

#7521 introduced TypedCollection for ExecuteProcessRequest arguments, and the added type strictness exposed a place where we weren't explicitly converting a binary string to a text string, failing the cron job's python 2.7 shard: https://travis-ci.org/pantsbuild/pants/jobs/518207912.

Solution

  • Use SubclassesOf(text_type) instead of Exactly(text_type) to accept any string arguments (fixing the error in rsc_compile.py in the noted cron job).
  • Explicitly convert the python executable path in the v2 pytest runner into a string.

Result

The cron job passes again, and people can put in whatever string type they like to ExecuteProcessRequest, as long as it's a text string.

@Eric-Arellano
Copy link
Contributor

left a comment

Is the reason for using Subclasses instead of Exactly so that future.builtins.str is accepted? I can't think of any other possible subclass of str.

If this is the case, I'd prefer we stick to Exactly because once we drop Py2 we wouldn't need the Subclass anymore and the solution would be more flexible than necessary. Instead, fix the callers of the function.

If you want to stick with Subclasses for now, but it will be possible to change to Exactly once we drop Py2 and future, then please add a TODO linked to this project https://github.com/pantsbuild/pants/projects/12.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Let's leave it as Exactly and just fix the env argument in the v2 pytest runner and the jvm options in rsc compile, those were the other type check failures in the failed cron job.

@stuhood
Copy link
Member

left a comment

Thanks!

Show resolved Hide resolved src/python/pants/engine/isolated_process.py Outdated
@Eric-Arellano
Copy link
Contributor

left a comment

Much better. Thanks!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-execute-process-type-constraints branch from 05701e0 to d52a702 Apr 11, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-execute-process-type-constraints branch from d52a702 to 802992e Apr 11, 2019

@cosmicexplorer cosmicexplorer requested review from blorente and ity Apr 11, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Thanks! Excited for being able to remove this cruft soon.

@Eric-Arellano Eric-Arellano referenced this pull request Apr 11, 2019

Merged

Prepare 1.16.0.dev1 #7535

@cosmicexplorer cosmicexplorer merged commit 8b33832 into pantsbuild:master Apr 11, 2019

1 check passed

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

jsirois added a commit to jsirois/pants that referenced this pull request Apr 14, 2019

jsirois added a commit to jsirois/pants that referenced this pull request Apr 14, 2019

jsirois added a commit that referenced this pull request Apr 15, 2019

Coerce argv for JavacCompile to text_type. (#7560)
This was a straggler from #7529.

Fixes #7559

cosmicexplorer added a commit that referenced this pull request Apr 24, 2019

fix cron job by wrapping the python binary in text_type() (#7611)
### Problem

The cron job is failing due to a type check error in an ExecuteProcessRequest creation: see https://travis-ci.org/pantsbuild/pants/jobs/522402485.

### Solution

- Wrap `sys.executable` in `text_type()` not `str()`.
  - This is necessary after the changes in #7529.

### Result

The cron job passes! This fix can be verified by running `./pants2 --print-exception-stacktrace --v2 --no-v1 test testprojects/tests/python/pants/dummies:target_with_source_dep` and seeing that it passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.