> On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote: > > cmake/CompilationConfigure.cmake > > Lines 308-311 (patched) > > <https://reviews.apache.org/r/66493/diff/1/?file=1993835#file1993835line308> > > > > I was briefly wondering why we checked for clang below. The reason is > > of course that cmake by default uses the compiler as linker, and there is > > an incompatibility on FreeBSD when compiling with clang and linking with > > `ld.bfd`. > > > > I think it would be useful to describe this in more detail here so we > > have a chance to understand when this check can go in the future. Ideally > > we'd be able to link some upstream issue, but it may not exist. > > Andrew Schwartzmeyer wrote: > Yeah, it took me a while to reason through this too, even with your > comment Benjamin. It's because of how the linker command is formed (described > [here](https://cmake.org/pipermail/cmake/2014-August/058268.html)). Not > knowing this will leave you bewildered here.
I'll make the comment a bit more descriptive. > On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote: > > cmake/CompilationConfigure.cmake > > Lines 315 (patched) > > <https://reviews.apache.org/r/66493/diff/1/?file=1993835#file1993835line315> > > > > This conditional seems not needed as cmake will automatically detect if > > `LD_PROGRAM` was set by the user. Is this a variable we should document as > > a customization point? > > Andrew Schwartzmeyer wrote: > Do you mean documenting like as an `option(...)` and in the docs, or just > in the docs? Maybe we should wait until the FreeBSD build is working, and > then add a doc for it (in which we could also explain the weirdness with > linkers). Hm, I didn't realize find_program wouldn't run if I set this on my own. I'll clean it up. As for documenting it, I figured that it would be best to wait until the build was fixed and settled. Even if someone is building on FreeBSD at this point nothing is really working. It might be a bit pre-mature to start surfacing docs about the build (which probably means there should be a ticket about documenting this?). > On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote: > > cmake/CompilationConfigure.cmake > > Lines 318 (patched) > > <https://reviews.apache.org/r/66493/diff/1/?file=1993835#file1993835line318> > > > > Since this variable will be setable from the outside _Alternative_ > > seems redundant here. Removed. > On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote: > > cmake/CompilationConfigure.cmake > > Lines 322 (patched) > > <https://reviews.apache.org/r/66493/diff/1/?file=1993835#file1993835line322> > > > > Could we make clearer what a user needs to do to have this check pass? > > The error message appears too technical to me. > > Andrew Schwartzmeyer wrote: > +1 I have no clue what I would do to get a "non-base LD." This isn't even accurate since lld is in base. I'll update it. > On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote: > > cmake/CompilationConfigure.cmake > > Lines 326-329 (patched) > > <https://reviews.apache.org/r/66493/diff/1/?file=1993835#file1993835line326> > > > > I am not sure this is an issue, but customary there are similar > > variables defined for other build types like `DEBUG`, `RELEASE`, etc. > > (e.g., `CMAKE_EXE_LINKER_FLAGS_DEBUG`). > > > > Do we have something to set all related flags @andschwa? > > Andrew Schwartzmeyer wrote: > It might soon be time to put this into a function, but yeah, it'd be a > nested for loop akin to this: > > ``` > foreach (lang C CXX) > list(APPEND CMAKE_${lang}_FORWARD_ARGS > ${CMAKE_FORWARD_ARGS} > -DCMAKE_${lang}_COMPILER=${CMAKE_${lang}_COMPILER} > -DCMAKE_${lang}_COMPILER_LAUNCHER=${CMAKE_${lang}_COMPILER_LAUNCHER} > -DCMAKE_${lang}_FLAGS=${CMAKE_${lang}_FLAGS}) > > foreach (config DEBUG RELEASE RELWITHDEBINFO MINSIZEREL) > list(APPEND CMAKE_${lang}_FORWARD_ARGS > -DCMAKE_${lang}_FLAGS_${config}=${CMAKE_${lang}_FLAGS_${config}}) > endforeach () > endforeach () > > ``` > > Note that this only matters if you're using a multi-configuration > generator (like Visual Studio and some other IDEs), but not `make` nor > `ninja`. Will switch over to something like that. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66493/#review200725 ----------------------------------------------------------- On April 10, 2018, 6:36 a.m., David Forsythe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66493/ > ----------------------------------------------------------- > > (Updated April 10, 2018, 6:36 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier. > > > Bugs: MESOS-8761 > https://issues.apache.org/jira/browse/MESOS-8761 > > > Repository: mesos > > > Description > ------- > > Made FreeBSD default to non-GNU ld. > > > Diffs > ----- > > cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c > > > Diff: https://reviews.apache.org/r/66493/diff/2/ > > > Testing > ------- > > make on FreeBSD, with both lld and gold. > > > Thanks, > > David Forsythe > >
