Makefile refactoring to factor out common build code #2672
Conversation
keeranroth
requested review from
Alexsandruss,
napetrov,
homksei,
ahuber21 and
ethanglaser
as code owners
keeranroth
changed the title
| @@ -0,0 +1,108 @@ | |||
| #=============================================================================== | |||
There was a problem hiding this comment.
Should we rename this to be aligned with ARM?
This is quite a legacy name as one of variants between 32bit, em64t and itanium64 variants.
Any ideas on what this would be? basically this is generic x86 architecture
There was a problem hiding this comment.
how about x86_64.mk?
There was a problem hiding this comment.
Agreed that this could be a better name, and x86_64 sounds decent. However, changing this requires updates outside of the make setup (there are ci scripts that depend on this value), and I think it would be better done as another merge request to change the name. What do you think? I'm happy to put something up for that after this.
There was a problem hiding this comment.
yes, x86_64 looks decent. - the only thing to check is _ handling and this would depend on how wide we would like to have this implemented, but in longerterm worth having better name.
yes, it's fine to keep this separate as renaming would require updates for internal CI. @homksei @ethanglaser - this is something that we can look on and rename legacy 32e naming to have better match/separation in future.
There was a problem hiding this comment.
#2674 as a draft merge request, for what this would look like on top of this commit.
Is there anything else that needs changing on my end for this merge request? I can see that all the public CI passes, but the Intel CI is failing
|
/intelci: run |
1 similar comment
|
/intelci: run |
|
/intelci: run |
|
Looks like another error. Are there some files that I missed the license text on, or is there something else? |
All (6) new files in function_definitions/ directory are failing copyright check. Will look into why this may be happening. Other copyright checks are passing (including files in compiler_definitions/) |
|
Hi @ethanglaser thanks for having a look at those. Is there anything that I can do on my side? Also, is it worth putting an updated copyright check into upstream as well? It would make it easier for me next time round to check this locally (I usually defer to CI checks before pushing a merge request) |
Match the same style as used in the rest of the script. I observed a syntax error when running in a Docker container locally as well, so fix this up quickly.
Following on from pull request oneapi-src#2614, we want to make it easier to distinguish common and platform specific code when building. We take common code from the main makefile, and split this out into arch and uarch specific files. We try and share as much code as possible between uarch files. In this first commit, we make sure that the x86 build works on linux. Next commits will make sure that the Mac, Windows and Arm builds also work. For now these are broken.
Put Mac and Windows makefile code into their own files, and correct some copy-paste errors in the Arm build. This should build on all platforms now, but there are some more commits to go on top before the refactor is done.
We want to have as much common code shared between makefiles for different compilers, as well as for the different backends. We shuffle things around a little. This now passes on x86 Linux as well as aarch64. Needs some internal tests to be run.
keeranroth
force-pushed
the
dev/keeranr/makefile-refactor
branch
from
e64a256
to
70eac50
Compare
|
Hi @keeranroth, it appear that the |
|
Good idea about adding copyright check to public CI - we'll discuss as a team and keep you updated. Apologies on if I missed discussion on this but did we agree on adding |
|
/intelci: run |
|
Ah, my bad on the copyright checks. @napetrov did say that the agreed style doesn't have a year in the statement, so I've updated those |
|
/intelci: run |
There was a problem hiding this comment.
Thanks for the reviews @ethanglaser , and sorry for not having seen this before. Must have missed the emails with your comments in.
I've left one point unaddressed regarding single vs double brackets in a bash script. Whilst I also have a preference for the double brackets, I changed this to match the existing style in the script. Anything else, let me know
|
/intelci: run |
Description
Following on from pull request #2614 to add Arm as a supported CPU platform, we update the Makefile structure to make it easier to add other platforms in the future.
The motivation behind this change is to have a minimal set of variables to change when adding a new platform. We also look to re-use makefile code as much as possible in the existing build system, so that it's less likely for builds on different platforms to start to diverge.
The changes to the makefiles require no changes to the build commands.
Most of the changes are copy and paste, with some changes being made to allow the evaluation of make commands to be deferred until all variables required are in scope. No new functionality is added, and no functionality is removed.
Changes proposed in this pull request: