-
-
Notifications
You must be signed in to change notification settings - Fork 102
Improved getDependency workflow #758
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
Improved getDependency workflow #758
Conversation
|
Logs :
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/55715/console
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/55707/consoleFull
|
|
I do not see that a
Only ran one test. It did not run entire sanity.functional. |
0f6efab to
f6657cc
Compare
|
Hi @llxia , As per discussion i have updated the script to consider non-system tests as well, special function : https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/55803/ Thanks |
scripts/getDependencies.pl
Outdated
| catch { | ||
| print "Warning : Download failed for $fn from custom url $url: $_"; | ||
| }; | ||
| if (!$download_success && $url ne "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to check $url here?
scripts/getDependencies.pl
Outdated
| my $full_dir_path = File::Spec->catdir($path, $dir); | ||
| my $url_custom = $customUrl; | ||
|
|
||
| my $thirdParty_Url = $url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use camel case or _, but do not mix them. In this case, call it third_party_url
scripts/getDependencies.pl
Outdated
| $download_success = 1; | ||
| } | ||
| catch { | ||
| print "Error: Failed to download $fn from third-party URL ($thirdParty_Url): $_"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the second download failed, the program should exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
scripts/getDependencies.pl
Outdated
| unless ($download_success) { | ||
| print " ERROR: Could not download $fn from either custom or third-party URL.\n"; | ||
| next; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed.
|
Please also run extended.functional. Since we are using a new library |
f6657cc to
672883a
Compare
|
Extended.functional : https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/55868/ Logs : |
dac6954 to
1163234
Compare
scripts/getDependencies.pl
Outdated
| $download_success = 1; | ||
| }; | ||
| if (!$download_success) { | ||
| print "Warning: Download failed for $fn from custom URL $url\nDownloading $fn from third-party URL: $third_party_url\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use $filename, not $fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
scripts/getDependencies.pl
Outdated
| $download_success = 1; | ||
| }; | ||
| if (!$download_success) { | ||
| print "Error: Failed to download $fn from third-party URL $third_party_url\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use $filename, not $fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
1163234 to
214c065
Compare
| use File::Copy; | ||
| use File::Spec; | ||
| use File::Path qw(make_path); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep this blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted blank lines added back
214c065 to
c8dde07
Compare
|
Could you fix |
c8dde07 to
3833722
Compare
LongyuZhang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
tested in parallel: extended.functional and sanity.functional @Amrutha-Kanhirathingal, please fix failed cases in sanity.functional. |
|
@llxia |
|
This PR should run on the master branch (not the release branch) to ensure all dependent tests are updated accordingly. |
|
@Amrutha-Kanhirathingal, |
|
|
||
| # define directory path separator | ||
| my $sep = File::Spec->catfile('', ''); | ||
| $customUrl = $ENV{'CUSTOM_URL'} if !defined($customUrl) || $customUrl eq ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New environment variable means local usr ( not jenkins story) has to do another export. Instead of setting in Jenkins file would it be better to set in makefile ie, evnSettings.mk or settings.mk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes Thanks for pointing this out.
https://github.com/adoptium/aqa-tests/blob/80a0985b5df03cbd05cebe9ae4a81a6a42bb8dfd/buildenv/jenkins/JenkinsfileBase#L215-L216
This is how we are fetching and assigning customUrl.
I’ll set the defaults in the Makefile as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sophia-guo ,
When testing locally, the jars are downloaded using the third-party URLs, so we do not need to set this environment variable in the Makefile. The current behavior already works for local runs without requiring any additional exports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is depends on another PR in aqa-tests, adoptium/aqa-tests#6694, which introduces a new environment variable in jenkins level. So for locals customUrl="" and env CUSTOM_URL is not set, so customUrl keeps empty. For jenkins, env CUSTOM_URL is set at pre stage and build.xml can use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes Absolutely
sanity.functional : https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/56773/console |
|
@Amrutha-Kanhirathingal You'll also need to rebase. |
smlambert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting on the update to mk file, as discussed in the review comments, so that this will work even when not run in a Jenkins environment.
Just to clarify my understanding: when running the tests locally, the jars are already downloaded using the third-party URLs, and the current workflow works without requiring any additional environment variable exports. Given this behavior, I wanted to confirm whether the change to the Makefile is still required, or if the existing setup is sufficient for non-Jenkins environments. Given that, it would be helpful if @Amrutha-Kanhirathingal could run a few local tests as well to confirm this behavior... |
|
Couple of notes, you need to test the case where when someone is running locally, the initial download has not run successfully. You may need to manually interrupt the download to set up that situation for testing. One more thing I would like changed around this PR is the environment variable name, which is very generic and not informative enough to tell what CUSTOM_URL, what is it for? Reading the issue that this PR is meant to fix, perhaps calling it DEPENDENCY_CUSTOM_URL, DEPENDENCY_FALLBACK_URL or something like that. |
|
Also, based on my reading of the comments above, is the implication that there is yet another parameter being set via Jenkins to set this environment variable on a test node? |
|
@sophia-guo - Can you reread the issue that describes what this intends to fix I am trying to understand the problem that this PR is intending to fix. If I read correctly, there are two issues being described, 1st issue was that getDependencies was downloading more dependencies than were needed. 2nd was that when larger file downloads fail, it retried, but this PR wants to have it retry from an alternate URL, (and if that fails, try fetching from the 3rd party URL)? |
|
Yes, this ENV{'CUSTOM_URL'} won't affect local run. Also this env variable was not introduced by this PR. I'm just in general not a fan of environment variable due to poor maintainability and reproducibility. |
Yes as I read the issue, @smlambert that your understanding is correct. The PR addresses both issues:
First attempt: download from the custom (test dependency job url ) Jenkins URL |
|
Based on adoptium/aqa-tests#6461 (comment), Lan's idea:
|
- getDependencies.pl script run twice.Once with customUrl. If it failed, retry with 3rd party URL. Closes: adoptium/aqa-tests#6461 Signed-off-by:Amrutha Kanhirathingal <Amrutha.Kanhirathingal@ibm.com>
3833722 to
bfb350f
Compare



Uh oh!
There was an error while loading. Please reload this page.