Power support #2139
Power support #2139
Conversation
|
Can one of the admins verify this patch? |
| if [ "${PLATFORM-}" = "darwin" ]; then | ||
| echo "Skipping test: linaro toolchain is not supported on darwin host." | ||
| if [ "${PLATFORM-}" = "darwin" -o "${MACHINE_TYPE}" = "ppc64le" ]; then | ||
| echo "Skipping test: linaro toolchain is not supported on darwin host or ppc64le Linux." |
There was a problem hiding this comment.
Can we change that test to check that platform is linux instead?
There was a problem hiding this comment.
After rebase, the test case started giving timeout error, even with checking linux instead of ppc64le or without any change i.e. just checking darwin. Is this a known problem? If it is, I'll undo this change and keep that original change of darwin.
There was a problem hiding this comment.
Our CI test is green so there is no issue. If the test is timing out on supported platform it might be because the platform is overloaded while running the test.
There was a problem hiding this comment.
Thanks, so should I change ppc64le check to "linux"? I've made that change but haven't committed it as the test was giving timeout. If it is green in your CI, I'll commit that change as you've suggested.
There was a problem hiding this comment.
Yes I mean this test is made to work only on Linux x86_64 so it's better to test that we are in the good architecture rather than list all the platform it cannot work on.
|
We will test this change after rebase. |
|
Jenkins test this please |
|
@damienmg Incorporated the requested change. Please review and let me know if any changes are required. |
| @@ -258,7 +258,7 @@ public void build() throws InterruptedException { | |||
| outputJar, javaStartClass, deployManifestLines, buildInfoArtifacts, classpathResources, | |||
| runtimeClasspath, includeBuildData, compression, launcher); | |||
|
|
|||
| List<String> jvmArgs = ImmutableList.of("-client", SINGLEJAR_MAX_MEMORY); | |||
There was a problem hiding this comment.
/cc @philwo: why do we still have -client flag on Java on some machine, is there case it would be needed?
|
I just have one nit left and pinged another developer to get a precision to know if it is safe to remove "-client" |
|
LGTM, I am merging it, if Klaus has no further objection the patch will be merged (Klaus away today so it will not be merged before monday). A last test from jenkins too: test this please. Thank you very much for your contribution and my apologies for the initial delay. |
|
@damienmg Thank you very much for your review and approval. |
| @@ -19,7 +19,7 @@ | |||
| * Various useful diagnostics functions from Linux err.h file, wrapped | |||
| * for portability. | |||
| */ | |||
| #if defined(__APPLE__) || defined(__linux) | |||
| #if defined(__APPLE__) || defined(__linux__) | |||
There was a problem hiding this comment.
How is this change related to supporting power? Also, are we sure the case defined(__linux) was wrong in the first place and is not needed any more?
There was a problem hiding this comment.
On Power, when we compile a program using -std=c++11, then __linux gets undefined, which otherwise is defined. However, on x86, -std=c++11 does not make any difference. I verified on x86 too, and defined(linux) is defined there as well.
On Power:
$ cpp -dM foo.c | grep linux
#define __linux 1
#define __linux__ 1
#define __gnu_linux__ 1
#define linux 1
$ cpp -dM -std=c++11 foo.c | grep linux
#define __linux__ 1
#define __gnu_linux__ 1
On x86:
$ cpp -dM foo.cc | grep linux
#define __linux 1
#define __linux__ 1
#define __gnu_linux__ 1
#define linux 1
~$ cpp -dM -std=c++11 foo.cc | grep linux
#define __linux 1
#define __linux__ 1
#define __gnu_linux__ 1
|
Could somebody point me to what is wrong with the test being failing on darwin platform as per CI build? Ideally the test should be skipped on darwin platform. |
| fi | ||
|
|
||
| if [$expected_platform -eq 0] | ||
| if ![ "${PLATFORM-}" = "linux" && "${MACHINE_TYPE}" = "x86_64" ]; then |
There was a problem hiding this comment.
Add a space between "!" and "["
There was a problem hiding this comment.
Oops, my bad! Thanks for pointing it out!.
|
Jenkins test this please |
|
Oops I should not have done that, the change is still missing |
|
Thanks @damienmg. I've pushed that change. |
|
Jenkins test this please |
Closes bazelbuild#2139. -- Reviewed-on: bazelbuild#2139 PiperOrigin-RevId: 142570236 MOS_MIGRATED_REVID=142570236
No description provided.