-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
zlinux micro architecture detection #477
Conversation
8be8ff7
to
ed415da
Compare
|
a082c65
to
1b8eb23
Compare
1b8eb23
to
ee9ddea
Compare
run tkg-test |
@annaibm TKG test build started, workflow Run ID: 7506926904 |
@annaibm Build(s) failed, workflow Run ID: 7506926904 |
ee9ddea
to
8707903
Compare
run tkg-test |
@annaibm TKG test build started, workflow Run ID: 7507448275 |
@annaibm Build(s) failed, workflow Run ID: 7507448275 |
28c29a2
to
b975ffb
Compare
run tkg-test |
@annaibm TKG test build started, workflow Run ID: 7561887930 |
@annaibm Build(s) failed, workflow Run ID: 7561887930 |
b975ffb
to
42d112b
Compare
run tkg-test |
@annaibm TKG test build started, workflow Run ID: 7562336648 |
d98fb7e
to
df2f2fc
Compare
run tkg-test |
@annaibm TKG test build started, workflow Run ID: 8573067259 |
@annaibm Build(s) successful, workflow Run ID: 8573067259 |
6790a68
to
a4d7dbe
Compare
run tkg-test |
@annaibm TKG test build started, workflow Run ID: 8634602513 |
@annaibm Build(s) successful, workflow Run ID: 8634602513 |
Grinder links: |
String microArchVersion = prSplitOnDot[2]; | ||
String environmentMicroArch = arg.getMicroArch(); | ||
if (microArchVersion.endsWith("+")) { | ||
int microArchVersionInt, environmentMicroArchVerInt; |
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 initialize the values
} | ||
return microArchVersionInt <= environmentMicroArchVerInt; | ||
} else { | ||
return environmentMicroArch.equals(microArchVersion); |
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 https://github.com/adoptium/TKG/pull/477/files#diff-1ce05e4d3d428c3c8430b54f2905415f30fd13919ff8269e99c127485d69093eR462-R499 as reference. For number comparison, please make a function if needed.
- We need to compare the first character as well. i.e., for arch.390.z15+, we need to ensure
z
matches.
bbcd29a
to
70aec5c
Compare
settings.mk
Outdated
@@ -103,7 +103,7 @@ ifneq (,$(findstring win,$(SPEC))) | |||
P=; | |||
D=\\ | |||
EXECUTABLE_SUFFIX=.exe | |||
RUN_SCRIPT="cmd /c" | |||
RUN_SCRIPT="cmd /c" | |||
RUN_SCRIPT_STRING=$(RUN_SCRIPT) | |||
SCRIPT_SUFFIX=.bat | |||
PROPS_DIR=props_win |
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.
There are no changes to this file other than formatting. I would prefer not to include this change. Please create separate PR for formatting if needed.
boolean isZCharMatch = environmentMicroArch.startsWith(microArchVersion.substring(0, 1)); | ||
boolean isZStart = microArchVersion.startsWith("z") && environmentMicroArch.startsWith("z"); | ||
if (!(isZCharMatch || isZStart)) { | ||
System.out.println("The microarchitecture does not match required starting character of 'z' prefix."); | ||
return false; | ||
} |
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 shuold not hardcode z
. What if we have other versions that start with different letter in the future? Please see suggested changed below.
String microArch = prSplitOnDot[2]; | ||
if (!microArch.equals(arg.getMicroArch())) { | ||
String microArchVersion = prSplitOnDot[2]; | ||
String environmentMicroArch = arg.getMicroArch(); |
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.
- rename microArchVersion to
requiredMicroArch
- rename environmentMicroArch to
actualMicroArch
@@ -484,4 +484,33 @@ private boolean matchPlat(String fullSpec, String pr) { | |||
} | |||
return true; | |||
} | |||
|
|||
private boolean isVersionCompare(String prVersion, String environmentVersion) { |
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 should be able to use one function. Something like:
private boolean executeTest(String requiredLabel, String actualLabel) {
if (requiredLabel.isEmpty() || actualLabel.isEmpty()) {
return false;
} else if (requiredLabel == actualLabel) {
return true;
} else if (requiredLabel.endsWith("+")) {
Pattern pattern = Pattern.compile("(\\D+)?(\\d+)");
Matcher requiredLabelMatcher = pattern.matcher(requiredLabel);
Matcher actualLabelMatcher = pattern.matcher(actualLabel);
if (requiredLabelMatcher.find() && actualLabelMatcher.find() && requiredLabelMatcher.groupCount() == 2 && actualLabelMatcher.groupCount() == 2) {
if (requiredLabelMatcher.group(1) == actualLabelMatcher.group(1)) {
int requiredLabelNum = 0;
int actualLabelNum = 0;
try {
requiredLabelNum = Integer.parseInt(requiredLabelMatcher.group(2));
actualLabelNum = Integer.parseInt(actualLabelMatcher.group(2));
} catch (NumberFormatException e) {
System.out.println("Error: unrecognized requiredLabel:" + requiredLabel + " or actualLabel:" + actualLabel);
System.err.println(e.getMessage());
System.exit(1);
}
if (actualLabelNum >= requiredLabelNum) {
return true;
}
}
}
}
return false;
}
70aec5c
to
6a2c35d
Compare
Thanks @annaibm . Please rerun tests with the latest change. And we should also test |
6a2c35d
to
5108976
Compare
Grinder links: OS testcases:
|
can you rerun this build? |
test_os_linux_ubuntu22 : https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/39872/(ubu22s390x-rt-1.hursley.ibm.com) |
- zlinux version detection - add tests Signed-off-by: Anna Babu Palathingal <[email protected]>
5108976
to
14de8ce
Compare
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
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
resolves: #471