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

Small logging update to java executor and fix undefined var in release.sh #7370

Merged

Conversation

Projects
None yet
3 participants
@cheister
Copy link
Contributor

commented Mar 12, 2019

Fixing undefined vars in release and better log messaging

@cheister cheister requested review from stuhood and Eric-Arellano Mar 12, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Cool! Would you be willing to add a test for this new branch please? I'm thinking you could either modify or emulate our util function sample_jarfile https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/backend/jvm/tasks/test_unpack_jars.py#L30 to test that this works for .tar.gz. We're highly unlikely to ever work with those files while doing Pants devs, so a test would be quite helpful to ensure we don't break its support.

@@ -469,7 +469,7 @@ function fetch_and_check_prebuilt_wheels() {
do
packages=($(find_pkg "${PACKAGE}" "${PANTS_UNSTABLE_VERSION}" "${check_dir}"))
if [ ${#packages[@]} -eq 0 ]; then
missing+=("${NAME}")
missing+=("${PACKAGE}")

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 12, 2019

Contributor

Is this change breaking anything for you at the moment, or you only noticed it and are being a good citizen? I can roll it into #7197 if you prefer Stu to keep this PR more focused.

This comment has been minimized.

Copy link
@cheister

cheister Mar 12, 2019

Author Contributor

It's not breaking anything at the moment, just needed to be cleaned up.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 12, 2019

Contributor

Sounds good! I'll add it to my PR that makes a bunch of releases.sh changes already, meaning it can be removed here and the PR title can be shortened.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 13, 2019

Contributor

So looking more into it, there are 5 distinct places we use ${NAME} in release.sh. I'm not entirely sure where it gets defined, but it seems like they're getting resolved as we run these functions when doing a manual release, and no one has seemed to notice.

I'm going to leave it out of my PR after all. If we still want to address it, I think this change would be best broken out into a dedicated PR.

@stuhood
Copy link
Member

left a comment

Thanks! It would be good to open a followup issue (or PR?) about renaming this to something more general.

One substantive comment.

if jar_path.endswith('.jar'):
ZIP.extract(jar_path, unpack_dir, filter_func=unpack_filter)
else:
archiver_for_path(jar_path).extract(jar_path, unpack_dir)

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 12, 2019

Member

This will not use the unpack_filter, but probably still should.

@@ -62,4 +62,7 @@ def unpack_target(self, unpacked_jars, unpack_dir):
if not unpacked_jars.payload.intransitive or coordinate in direct_coords:
self.context.log.info('Unpacking jar {coordinate} from {jar_path} to {unpack_dir}.'.format(
coordinate=coordinate, jar_path=jar_path, unpack_dir=unpack_dir))
ZIP.extract(jar_path, unpack_dir, filter_func=unpack_filter)
if jar_path.endswith('.jar'):

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 12, 2019

Member

So, assuming that the unpack_filter can be applied to archiver_for_path (I think it needs to be...) you shouldn't need this conditional, and can just always use archiver_for_path.

@cheister cheister force-pushed the cheister:unpack-non-jars-and-small-cleanup branch from 6384e0c to 748b3f2 May 2, 2019

@cheister cheister changed the title Allow unpack jars to work with tar.gz files, small logging update to java executor and fix undefined var in release.sh Small logging update to java executor and fix undefined var in release.sh May 2, 2019

@cheister

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Removed the changes to unpack_jars.py

@stuhood

stuhood approved these changes May 2, 2019

@cheister cheister merged commit db391a8 into pantsbuild:master May 2, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@cheister cheister deleted the cheister:unpack-non-jars-and-small-cleanup branch May 2, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

I think that this broke CI due to the changes to release.sh. The Linux build wheels shards consistently time out: https://travis-ci.org/pantsbuild/pants/jobs/527551056#L1436.

It looks like this PR was merged on red CI, which is why we didn't catch this.

@cheister could you please put up a PR to revert the changes to release.sh? We should keep the rest of the changes.

stuhood added a commit to twitter/pants that referenced this pull request May 3, 2019

@stuhood stuhood referenced this pull request May 3, 2019

Merged

Revert #7370 #7656

stuhood added a commit that referenced this pull request May 3, 2019

Revert #7370 (#7656)
This reverts commit db391a8.

cheister pushed a commit to cheister/pants that referenced this pull request May 6, 2019

stuhood added a commit that referenced this pull request May 7, 2019

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.