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

HADOOP-17891. Fix compilation error under skipShade (ADDENDUM) #3441

Merged
merged 2 commits into from
Sep 16, 2021

Conversation

viirya
Copy link
Member

@viirya viirya commented Sep 15, 2021

This is a follow up for #3385 to fix the compilation error under skipShade.

@viirya viirya changed the title HADOOP-17891. HADOOP-17891. Fix compilation error under skipShade Sep 15, 2021
@viirya
Copy link
Member Author

viirya commented Sep 15, 2021

cc @sunchao

@viirya viirya changed the title HADOOP-17891. Fix compilation error under skipShade HADOOP-17891. Fix compilation error under skipShade (ADDENDUM) Sep 15, 2021
@sunchao
Copy link
Member

sunchao commented Sep 15, 2021

cc @ayushtkn too

@hadoop-yetus
Copy link

(!) A patch to the testing environment has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3441/1/console in case of problems.

@sunchao
Copy link
Member

sunchao commented Sep 15, 2021

@viirya I think you should use -DskipShade instead of -PskipShade. The test still doesn't run properly.

@viirya
Copy link
Member Author

viirya commented Sep 15, 2021

Oops! Fixed. Thanks.

@hadoop-yetus
Copy link

(!) A patch to the testing environment has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3441/2/console in case of problems.

@apache apache deleted a comment from hadoop-yetus Sep 16, 2021
@viirya
Copy link
Member Author

viirya commented Sep 16, 2021

Seems okay now. patch-shadedclient.txt was passed.

@viirya
Copy link
Member Author

viirya commented Sep 16, 2021

The -1 from shadedclient is for branch-shadedclient.txt. But why branch build uses the updated dev-support/bin/hadoop.sh too?

Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build failure is just demonstrating what was broken.
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3441/2/artifact/out/branch-shadedclient.txt

It is fixed post the patch.
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3441/2/artifact/out/patch-shadedclient.txt

The -1 from shadedclient is for branch-shadedclient.txt. But why branch build uses the updated dev-support/bin/hadoop.sh too?

Yahh, I know would be a bit annoying for you. But it is build like that...

Anyway, That won't block this from getting in. Tried Locally as well. Things work now.
Thanx @sunchao and @viirya for fixing this.

@viirya
Copy link
Member Author

viirya commented Sep 16, 2021

Thank you @ayushtkn !

Copy link
Member

@sunchao sunchao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Thanks @viirya and @ayushtkn !

<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-common</artifactId>
<scope>test</scope>
<type>test-jar</type>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: actually we may merge this with line 173

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is type test-jar. Can we merge with type jar (line 173)?

Copy link
Member

@iwasakims iwasakims Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked as far as I tried on #3447, while I think it would be ok to have dedicated section for both jar and test-jar side-by-side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it looks fine to me too.

Copy link
Member

@iwasakims iwasakims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, pending @sunchao's comment.

@sunchao sunchao merged commit a424878 into apache:trunk Sep 16, 2021
@sunchao
Copy link
Member

sunchao commented Sep 16, 2021

Merged to trunk. Thanks @viirya !

@viirya
Copy link
Member Author

viirya commented Sep 16, 2021

Thank you @sunchao @ayushtkn @iwasakims !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants