-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
WIP: CMake #11754
WIP: CMake #11754
Conversation
👍 💯 I'll try this out tonight |
Looks cool. What are the advantages (or future ones) ? I don't know much about buildsystems but from the outside what we have now seems to work well. If it eases up the maintenance for people actually making this work then of course all in :-) Maybe the only problem I ever encounter is parallel build from scratch sometimes fails on dependencies (but restarting fixes it). |
I don't think rolling our own will be maintainable forever. LLVM plans to deprecate autotools entirely in another version or two. I think the right immediate path forward for this is to add it as an alternative option coexisting with the makefiles for a while (use the snippet from #9422 to disallow in-tree builds, so this doesn't overwrite the existing makefiles), until people get used to it. |
I totally agree with you. In fact, that's the main reason I've been struggling whether to implement this even though I'm personally a cmake fan. It should provide certain benefit (see @tkelman 's comment above and I'll also list some of mine below) but it's quite boring to do... (It's not that hard but can get quite boring after a while, which is, btw, when I decided to push it out...) A few benefit that I can think of
A few possibly down side
|
Oh and MSVC support (which is convered by @tkelman 's comment). Just want to mention here because that's the argument I hear a lot. (Although personally I couldn't care less...) |
With spec file macros or an equivalent toolchain file, cross-compilation in cmake works fine. I've used it for Lapack, Metis, HDF5, AMPL-MP, and others on the opensuse build service, it's not that bad. More verbose than The custom-commands-with-side-effects has gotten better in recent versions of cmake, there are some new features for that. Though you can possibly get into trouble on old distros if you require a feature that's only in the newest cmake - there are PPA's or generic binary installs of cmake that work though. This is tedious manual work to port over makefiles into cmake and @yuyichao is a saint for putting the work into it. Once we can start using |
and
Yes that's basically the plan. I've never personally used external_project but I saw projects that uses it and works.
The pattern I'm using is this. I don't have a old cmake to test it though. I also have some functions for download/untar with the expected behavior here and I'll bring those if necessary. (That file is GPL but I'm the author so don't worry about it...) |
@tkelman I was trying how to get cmake to link the sysimg object file to a shared object file. Can you check if this trick works with MSVC when you have time? |
Another thing is the generation of |
I think it'll need to be using a newer LLVM than 3.3 to emit comdat sections for the MSVC linker to accept it, and I don't have an MSVC build of recent LLVM handy. Based on some recent commits to master (3a63b2a) I think @vtjnash may have been doing something related to using the MSVC linker but whatever he was doing wasn't exactly openly-worked-on through an issue/PR. I suspect that was more about linking a mingw-built sys.dll into an msvc-built application though, otherwise he probably would have also fixed the alignment segfault in |
That's a pack of issues with MSVC that I've never heard of before.... Anyway, I'm not seeking MSVC support in this PR and I feel like this PR should be post 0.4 (or even post Arraymageddon depending on how long it takes). |
Agreed, MSVC support does not need to be fully working within the scope of this PR. I haven't even opened a separate issue for the |
it wasn't anything related to msvc, sorry. i just sometimes get asked to do unusual tests like 366740a to certain use cases more reliable (or in that case, to fail with certainty on any pointer bugs). this was inspired by the default behavior on OS X as documented in http://www.unix.com/man-page/osx/1/ld/ i've never attempted a msvc build, although it might make some aspects of debugging / line numbers easier if we could get it working. |
c9249d7
to
56e67f4
Compare
endif() | ||
elseif(CMAKE_C_COMPILER_ID STREQUAL "GNU") | ||
jl_set_option(USEGCC On) | ||
add_definitions(-pipe -fPIC) |
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.
-fPIC
gives a warning with mingw, that's why it's set to empty on windows
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.
I see. These compiler flags are far from complete. I'm planing to do a more careful port of the flags after the infrastructure is there.
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.
Just curious. What warning does it give?
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.
../Source/umf_symbolic_usage.c:1:0: warning: -fPIC ignored for target (all code is position independent) [enabled by default]
/* ========================================================================== */
^
Harmless, but annoying if it happens on every file.
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 is probably one of the stupidest warning I've seen.....
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 is a CMAKE_POSITION_INDEPENDENT_CODE
variable in cmake (and the corresponding POSITION_INDEPENDENT_CODE property for targets) which should handle this.
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.
Thanks. Will use that.
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.
BTW, this is cleared for mingw
or do I still need to check that.
Collecting some todo's from me as a comment instead of inline so github doesn't eat them on a rebase:
|
Actually for breaking makefile, can we rename the current Makefiles to |
NVM doesn't work on case insensitive system.... = = ............................... |
if(WIN32) | ||
set(libsupport_SRCS ${libsupport_SRCS} asprintf.c) | ||
if(CMAKE_SIZEOF_VOID_P EQUAL 8) | ||
set(libsupport_SRCS ${libsupport_SRCS} _setjmp.win64.c _longjmp.win64.c) |
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.
these are assembly, .S
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.
Ooops.
BTW let's move the discussion to the main comment. It's easier to get lost and I find the main comment easier to find. We can link to the code if necessary. |
So the part I would like to keep is basically this, which I think is pretty straightforward. The logic in |
That foreach, on its own, makes sense for dealing with timestamps. The automatic generation of dependency file lists doesn't, it's too confusing and indirect. |
Do you mean, how the file is generated? how it is read? why it is needed? |
All of the code for dealing with those lists is currently confusing. I can see roughly what you're aiming for, but I think it's total overkill for now.
Sure. And I think that's just fine - we say that you have to re-run the cmake configure step (or do |
I guess you are confused by the autodeps file. |
And you'd be right about it. I'm actually using that file as a time stamp to rerun the command when the compilation is aborted. I totally agree that needs to be changed. For the autodeps you can just replace it with the one without the suffix and it should still works. |
That and the |
And wouldn't setting |
6c3d5d1
to
c8a6b62
Compare
Given the discussion in #8745 (comment) it sounds like we might need to implement the checksumming/listing of source files included during sysimg build anyway, either as a separate file or embedded into a data section of the shared library. So I'm not as opposed to that now as I originally was. |
Yeah, I noticed that and I'm just going to wait for a decision to be made there before I replace what I have here with the new mechanism. This might mean that I could not have a generic version of it and rather a sysimg specific macro but that's totally find and will make the wrapper easier to understand as well. |
Actually now I'm thinking generic is good, we could install a few cmake definitions in share or something that teach cmake how to build packages or other Julia modules. That would solve the build system issue, avoiding reinventing make by instead just using cmake. But for now the build system checksumming/timestamping parts of the question are not the problem that PR is trying to address. |
By "generic" here I mean apply to other kind of targets as well (i.e. not only julia sysimg/module compilation), which is probably not what you want.
Yeah. these helper functions will be in a separate file and figuring out how to reuse them can be done later. |
I'm closing this one since there doesn't seem to be an immediate need for it, and probably needs new work anyways given how old it is. |
So after struggling for a few month, I finally decided to do the cmake port myself during the free time of a physics conference. A few things that I want to mention.
make
for the part I've not ported yet. It does have the advantage that the build is somewhat functional as I'm porting it.In it's current state,
julia
excutable works. Dependencies and sysimg are still calling the old makefile@tkelman
Ref #9422 #11645 #1832