Better support building the runtime separately #4743
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,8 @@ else() | |
| list(REMOVE_ITEM testnames uuid) | ||
| endif() | ||
|
|
||
| string(REPLACE ";" " " LDMD_CMD "${LDMD_EXE_FULL} ${D_EXTRA_FLAGS}") | ||
|
There was a problem hiding this comment. Your use case would be setting AFAICT, the multilib case would be designed to be used with the MODEL make variable: ldc/runtime/druntime/test/common.mak Line 33 in 1f0c36e The MODEL would make it into DFLAGS: ldc/runtime/druntime/test/common.mak Line 41 in 1f0c36e and CFLAGS_BASE (which we override for non-MSVC targets via CMake ldc/runtime/druntime/test/common.mak Line 34 in 1f0c36e How do you currently handle the C side for the separate 32-bit build? Setting a CC env var incl. an There was a problem hiding this comment.
Yes, only
These variables are set already by the Gentoo helper functions so I didn't have to interact with them but, if I followed the functions right:
Here's what the logs contain: So yeah, setting Setting
But can you explain something for me? Does main: main.cpp
$(CC) $(CFLAGS) -o main main.cpp
There was a problem hiding this comment. I seem to recall/assume (haven't tested it now!) that on Windows, you are supposed to use explicit quotes, something like Edit: Tested it; the cmdline needs extra quoting, but the rest holds: There was a problem hiding this comment. Amazing! I don't have a windows machine so I trust you on these. So, what's the final solution we go with?
1 is unintutive on windows because of the comamnd magic, 2 is unintuitive everywhere since the variables don't really seem related. If the tests are adjusted to also test for 32 bit with 1 + MODEL: # do_druntime_tests invokes the tests with make MODEL=${1}
do_druntime_tests("")
if(MULTILIB)
do_druntime_tests("${HOST_BITNESS}")
endif()1 + MODEL_FLAG: # do_druntime_tests invokes the tests with make MODEL_FLAG=${1}
do_druntime_tests("")
if(MULTILIB)
do_druntime_tests("-m${HOST_BITNESS}")
endif()2 + MODEL_FLAG: # do_druntime_tests invokes the tests with make MODEL_FLAG="${D_EXTRA_FLAGS} ${1}"
do_druntime_tests("")
if(MULTILIB)
do_druntime_tests("-m${HOST_BITNESS}")
endif()There was a problem hiding this comment. I have to, sometime, change the dmd makefiles to allow taking flags from the command line instead of relying on hacks like using MODEL_FLAG, as Gentoo policy dictates it. If I get to upstream these changes we should be able to do There was a problem hiding this comment.
AFAIK, it's common to have such CFLAGS_BASE, in order to only set optional extra/critical flags on demand. Specifying a make variable like this (after Similarly, ldc/runtime/druntime/test/common.mak Line 56 in 1f0c36e There was a problem hiding this comment.
[The non-super-consistently named CMake variables come from having a fat umbrella CMake build for LDC, where runtime and Phobos are just a subset of all builds. So we ended up with some ugly names for compiler-only D flags, runtime-only D flags etc. etc. They are usually empty, but designed exactly for such target-specific critical flags and/or custom flags.]
The only potential problem I see with you specifying the C flags explicitly this way is that they would be duplicated when building the runtime libraries (due to Gentoo's CMake hook injecting the critical flags), and potentially also for the make integration tests if CC was still set with flags at test-time. The compiler might complain about some options being specified multiple times. There was a problem hiding this comment.
I meant that I would pass There was a problem hiding this comment. Yeah that's the case we can handle. But what about the actual library build? There was a problem hiding this comment. Yeah, in that case the flags would be duplicated. This shouldn't be an issue and, even if it is, I can always just patch the cmake files to use a separate variable for passing abi-related CFLAGS and only respect in when invoking make, like This is slightly more work for me but, given the choices:
I would prefer for ldc to use 2, because, if it breaks, it would be easier to diagnose even if that entails me possibly carrying one more patch downstream. |
||
|
|
||
| foreach(name ${testnames}) | ||
| foreach(build debug release) | ||
| set(druntime_path_build ${druntime_path}) | ||
|
|
@@ -72,7 +74,7 @@ foreach(name ${testnames}) | |
| ) | ||
| add_test(NAME ${fullname} | ||
| COMMAND ${GNU_MAKE_BIN} -C ${PROJECT_SOURCE_DIR}/druntime/test/${name} | ||
| ROOT=${outdir} DMD=${LDMD_CMD} BUILD=${build} | ||
| DRUNTIME=${druntime_path_build} DRUNTIMESO=${shared_druntime_path_build} | ||
| SHARED=1 ${cflags_base} ${linkdl} | ||
| ) | ||
|
|
||
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.
And yet we add a
add-multilib-sectiontarget name 3 lines below. According tothis should still make sure the target is created before building any dependent. The (additional) path dependency should take care of the 2nd part wrt. rebuilds IIUC:
I guess they actually mean 'if the target is an add_executable or add_library target', so that one has to specify the path to the
add_custom_targetexecutable manually for proper rebuilds.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.
From looking at
build.ninja, theadd-multilib-sectiontarget is added as a dependency to the final libraries, likelib64/libdruntime-ldc-shared.so.109.1. I think this is fine, as the libraries are compiled with-conf=so they don't actually need the config file to be updated. In this case I think the weird cmake behavior does what we want: "update the config file before declaring the library built", it just does it a little bit later in the process.I'm reading this the other way around:
add_custom_commandoradd_custom_target.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'm pretty sure the 'target' they refer to is the dependency being added, i.e.,
LDC_EXE(expands toldc2) here. Which can either be anadd_executableor anadd_custom_target, depending onLDC_LINK_MANUALLY. So in case the ldc2 executable is a custom target, we additionally addLDC_EXE_FULLso that the runtimes stuff is rebuilt whenever the compiler is.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.
Ah, yes, I understand it the same way. Maybe I failed to word it right but, in my defense, it is a unintuitive behavior cmake is exhibiting.
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.
Fully agreed wrt. unintuitive (edit: and bad wording on my part - the compiler file dependency via LDC_EXE_PATH should make sure existing runtime build artifacts get stale as soon as the compiler is rebuilt). Probably related: we had CI issues with the
makebackend, IIRC some targets not waiting for the prerequisites to be finished. Working fine withninjathough.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.
Okay after another re-read, I think I got your point now - that the
LDC_EXEtarget name dependency does NOT apply to the custom command getting this dependency directly, but only to the dependents of that custom command. So you're absolutely right, the target-name dependency is insufficient, we need the path dependency to make it apply to the custom command directly, and not just for rebuilds. Thanks for digging!