build: Set noarch as default TARGET_ARCH #1947
Conversation
This will help for debian upgrade Change-Id: I2991d324b887e340cb676f7de8ea0dda2ea7c050 Forwarded: jerryscript-project#1947 Bug: jerryscript-project#1945 Bug-Debian: https://bugs.debian.org/957364 Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/review/master IoT.js-DCO-1.0-Signed-off-by: Philippe Coval rzr@users.sf.net
rzr
force-pushed
the
sandbox/rzr/cmake/review/master
branch
from
68a5d9d
to
e122c37
Compare
This will help for debian upgrade Change-Id: I2991d324b887e340cb676f7de8ea0dda2ea7c050 Forwarded: jerryscript-project#1947 Bug: jerryscript-project#1945 Bug-Debian: https://bugs.debian.org/957364 Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/review/master IoT.js-DCO-1.0-Signed-off-by: Philippe Coval rzr@users.sf.net
This will help for debian upgrade Change-Id: I2991d324b887e340cb676f7de8ea0dda2ea7c050 Forwarded: jerryscript-project#1947 Bug: jerryscript-project#1945 Bug-Debian: https://bugs.debian.org/957364 Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/review/master IoT.js-DCO-1.0-Signed-off-by: Philippe Coval rzr@users.sf.net
| if(NOT DEFINED TARGET_ARCH) | ||
| message(WARNING "Use generic flags since TARGET_ARCH is not defined") | ||
| set(TARGET_ARCH "noarch") | ||
| endif() |
There was a problem hiding this comment.
If we are adding a TARGET_ARCH to be defined as "noarch" if it is not defined then the last else in the following check will never trigger. Is it not possible to set this via debian rules?
There was a problem hiding this comment.
well I used to overide in debian/rules but I thought it would be better to have it set default upstream to avoid relying on undefined values,
I could remove the else warning since it's added in my patch.
There was a problem hiding this comment.
ohh wait. I misunderstood the if-s here. The change is ok. If the user specified an incorrect arch then a warning will be printed out bellow, but if there is no arch configured then this will be triggered.
| if(NOT DEFINED TARGET_ARCH) | ||
| message(WARNING "Use generic flags since TARGET_ARCH is not defined") | ||
| set(TARGET_ARCH "noarch") | ||
| endif() |
There was a problem hiding this comment.
ohh wait. I misunderstood the if-s here. The change is ok. If the user specified an incorrect arch then a warning will be printed out bellow, but if there is no arch configured then this will be triggered.
|
yes this is same as other board patch |
There was a problem hiding this comment.
LGTM (with a minor comment)
Works with me, for now, since noarch is already being used in our build terminology. But it is used incorrectly, in my opinion. At least, it is not consistent with the more widespread meaning of "noarch". In rpm/deb world, noarch usually labels a package that is not compiled, doesn't contain any binaries. A "noarch" package usually contains docs or scripts only. So, here, noarch is misleading IMO.
I think "generic" would be a better value for such a TARGET_ARCH, as that would trigger a build with no architecture-specific compiler options -- but that's still a build.
However, that would need a patch that touches more files in the code base, so that should/could happen in a follow-up.
This will help for debian upgrade
Change-Id: I2991d324b887e340cb676f7de8ea0dda2ea7c050
Forwarded: https://github.com/jerryscript-project/iotjs/pull/rzr
Bug: #1945
Bug-Debian: https://bugs.debian.org/957364
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/review/master
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval rzr@users.sf.net