Better support building the runtime separately [mod] #4752

kinke · 2024-09-12T17:02:11Z

Based on #4743.

For custom commands that require a built ldc2 specify only an absolute path to compiler binary, not the target name. This is because cmake handles file paths and target dependencies differently. In the case of a target dependency the dependency doesn't actually apply to the `add_custom_command`, it only affects future targets. As an example, we want to built libruntime.a from a D file foo.d. What we need to describe is: 1. compile foo.o from foo.d (depends on LDC) 2. archive libruntime.a from foo.o If we use ${LDC_EXE} as a dependency (only the target name) cmake would generate: 1. compile foo.o from foo.d 2. archive libruntime.a from foo.o (depends on LDC) Using an absolute path for the LDC dependency does what we want it to do. Signed-off-by: Andrei Horodniceanu <[email protected]>

Signed-off-by: Andrei Horodniceanu <[email protected]>

D_EXTRA_FLAGS is the intended way to pass flags like -m32 to ldc so make sure that its value is respected during the druntime tests. Signed-off-by: Andrei Horodniceanu <[email protected]>

…ke-based integration tests]

the-horo · 2024-09-12T20:42:44Z

Look good. With the last commit we respect RT_CFLAGS when invoking the druntime tests which is always a plus.

Other objectives that we may want to achieve, mostly taken from the discussion in the previous PR.

1. respect configure-time CC and CXX

The current makefiles will use the default values from the environment when the tests are run for the C and C++ compiler, basically ignoring what cmake found, or was configured with. This could be fixed by calling make with CC=${CMAKE_C_COMPILER} and CXX=${CMAKE_CXX_COMPILER}

2. (why we can't) use CC to pass arguments

While gmake should cope with CC being a command and arguments (gcc -m32, zig cc, C:\Program Files\my cc.exe -m32, all work) ldc can not. On windows ldc will treat CC literally, respecting spaces, making it possible to specify CC=C:\Program Files\my cc.exe but it will break with C:\Program Files\my cc.exe -m32.

We have no easy way to fix this as changing the driver code is not feasible so we can only declare a separate variable to hold the arguments of CC. To make it easier to follow the variables let's put these arguments in CC_ARGS.

This story only applies to CC, CXX and DMD aren't inspected by ldc so their requirement are only the ones imposed by gmake, i.e. a command and possible arguments. We can discuss if we should do with those the same we are forced to do for CC, so having DMD_ARGS and CXX_ARGS, or using the make convention and passing arguments inside them.

The current code in cmake is closest to using *_ARGS as it already makes the distinction of splitting $(CC) into a ${CMAKE_C_COMPILER} and ${RT_CFLAGS}.

3. divergence of the makefiles from the standard way of passing flags

This is mostly an issue with the makefiles but I think it is still worth discussing as changing the makefiles can benefit us by simplifying the logic we need to carry out.

This is how a make rule for compiling a C program should look like: https://www.gnu.org/prep/standards/html_node/Command-Variables.html#Command-Variables

foo.o: foo.c
     $(CC) -c $(CPPFLAGS) $(target_specific_flags) $(CFLAGS) $<

We don't use CPPFLAGS so we can continue to ignore it. I already explained why we need $(CC) to be split in command and arguments. With these changes our rules should look like:

foo.o: foo.c
     $(CC) $(CC_ARGS) -c $(target_specific_flags) $(CFLAGS) $<

What is important in the above command:

CFLAGS is the preferred way of passing user flags
CFLAGS is freely modifiable, the target has private flags that make sure that it can be built. CFLAGS are just an addition.

Note that it's also acceptable to have:

MY_FLAGS = -Imy_dir -DFOO_BAR $(CFLAGS)
foo.o: foo.c
     $(CC) $(CC_ARGS) -c $(MY_FLAGS) $<

as this is leads to the same command being run.

How does the current makefiles handle these flags:

It uses the standard name CFLAGS for passing its internal flags. Does this affect us that much? Maybe, maybe not. The standards would help if other people had to contribute to the project as the variables would carry a similar meaning to most other projects. Unless there is a good reason for why we won't follow the standard, example being our CC situation as our code would not work on windows if CC contained arguments, I see no point in purposely using the wrong variable name. We can rewrite the makefiles to be more compliant but that takes work so, in the meantime, we can at least try not to introduce more wrongness.
It doesn't have a variable to put arbitrary flags in. This leads to us working against ourselves my modifying variables that we set inside the makefiles, possibly leading to issues with important flags being dropped. Well, technically MODEL_FLAG is available but that is a implementation detail of the makefile so it it's like modifying private variables, not a sign of good code.

What I propose as a fix

1 should be straightforward. So long as there is a RT_CFLAGS or similar variable to pass flags down to CC everything should work.

2 depends on how we want to pass the flags. If we decide to pass RT_CFLAGS as CFLAGS in the rule below:

     $(CC) $(CC_ARGS) -c $(target_specific_flags) $(CFLAGS) $<

we may not need to implement CC_ARGS at all as we won't be using them. If we want to pass RT_CFLAGS in CC_ARGS then we would need to implement support for this. ditto for DMD and CXX but with those we can also pass arguments inside $(DMD) and $(CXX).

3 is the hardest as what I see as the proper solution would require, significantly, rewriting the makefiles. If we want to go with a bandage solution we should at least introduce variables for specifying arbitrary flags, separate from all the variables that are currently in use and make sure that their values are respected in all invocations of the corresponding compiler. In short, I would prefer if

ldc/runtime/DRuntimeIntegrationTests.cmake

Line 82 in 7f31303

CFLAGS_BASE=${cflags_base} DFLAGS_BASE=${dflags_base} ${linkdl}

used a separate variable for passing down CFLAGS.

kinke · 2024-09-13T11:49:08Z

respect configure-time CC and CXX

Perfect, that's what I wanted in the first place (and good to include CXX too), but was afraid of breaking your use case of test-time CC env variable. But you're okay with that in #4743 (comment), perfect.

(why we can't) use CC to pass arguments

Wrt. LDC on Windows not supporting a CC env var with flags - I don't think that'd be a deal breaker. E.g., the current proposal here converts the CMake semicolon-separated lists to space-separated lists via a char-replace, so flags containing spaces aren't quoted and would (likely) break the commands. I think it's fine to focus on Posix first, especially wrt. separate runtime builds and extra target-specific flags and wanting to run the druntime integration tests, on Windows.

divergence of the makefiles from the standard way of passing flags

Yeah that's probably an upstream discussion, but I don't see it as a problem. The actual test Makefiles simply want to use a single combined {C,D}FLAGS variable to keep things simple. And the C[++] stuff is hardly used, there are only few interop tests (shared and stdcpp):

CFLAGS is used in a single test Makefile, with 4 occurrences. CFLAGS_BASE is unused.
CXXFLAGS is unused, but CXXFLAGS_BASE is used in another single test Makefile, with 4 occurrences.
DFLAGS is ubiquitous, with some 72 occurrences in test Makefiles.

Edit:

CC is used in both 'interop' test Makefiles, a total of 9 usages.
CXX in the stdcpp Makefile only, 5 usages.

…tion tests] To propagate the C[++] compilers at CMake configure-time to test-time.

kinke · 2024-09-13T13:04:11Z

runtime/CMakeLists.txt

 cmake_minimum_required(VERSION 3.4.3)
+project(runtime C ASM CXX) # CXX for integration tests only


Without this, ${CMAKE_CXX_COMPILER} was empty for a separate runtime build.

FWIW, I've tested this with bin/ldc-build-runtime --ldcSrcDir=$PWD/../ldc --ninja --dFlags="-mtriple;my-triple" --cFlags="-target;my-triple" and looking at ldc-build-runtime.tmp/CTestTestfile.cmake:

add_test(druntime-test-aa-debug "/usr/bin/gmake" "-C" "/home/mkinkelin/dev/ldc/runtime/druntime/test/aa" "ROOT=/home/mkinkelin/dev/ninja-ldc/ldc-build-runtime.tmp/druntime-test-aa-debug" "DMD=" "BUILD=debug" "SHARED=1" "CC=/usr/bin/cc" "CXX=/usr/bin/c++" "DRUNTIME=/home/mkinkelin/dev/ninja-ldc/ldc-build-runtime.tmp/lib/libdruntime-ldc-debug.a" "DRUNTIMESO=/home/mkinkelin/dev/ninja-ldc/ldc-build-runtime.tmp/lib/libdruntime-ldc-debug-shared.so" "CFLAGS_BASE=-target my-triple -Wall -Wl,-rpath,/home/mkinkelin/dev/ninja-ldc/ldc-build-runtime.tmp/lib" "DFLAGS_BASE=-mtriple my-triple" "LINKDL=-L-ldl")

DMD is empty, ldc-build-runtime defines LDC_EXE_FULL, but not LDMD_EXE_FULL; will fix. Edit: Fixed.

Without this, ${CMAKE_CXX_COMPILER} was empty for a separate runtime build.

Yeah, that's expected. We need need the CXX language to be added to the project if we use a C++ compiler, even if only during the tests.

FWIW, I've tested this with bin/ldc-build-runtime --ldcSrcDir=$PWD/../ldc --ninja --dFlags="-mtriple;my-triple" --cFlags="-target;my-triple" and looking at ldc-build-runtime.tmp/CTestTestfile.cmake:

My build + tests worked fine as well.

kinke · 2024-09-13T13:37:58Z

Oh god, now hitting that super-weird CMAKE_C_COMPILER issue on Apple (some deeply nested usr/bin/cc which fails to find any system library apparently) when forwarding it to make: #3901

the-horo · 2024-09-13T14:25:51Z

Oh god, now hitting that super-weird CMAKE_C_COMPILER issue on Apple (some deeply nested usr/bin/cc which fails to find any system library apparently) when forwarding it to make: #3901

Lovely. The only thing I got from the discussion is that -lpthread is part of some global System library so I don't think I can help with it.

The windows tests are also failing:

C:/Program Files/LLVM/bin/clang-cl.exe /Wall /O2 /FeD:/a/ldc/build/runtime/druntime-test-shared-release/linkD.exe /FoD:/a/ldc/build/runtime/druntime-test-shared-release/linkD.exe.obj src/linkD.c D:/a/ldc/build/runtime/druntime-test-shared-release/lib.lib 
clang-cl: error: cannot specify '/FoD:/a/ldc/build/runtime/druntime-test-shared-release/linkD.exe.obj' when compiling multiple source files

From a quick glance over the ms docs I think this is fixable. It seems I can't directly make a suggestion https://github.com/orgs/community/discussions/9099:

diff --git a/runtime/druntime/test/shared/Makefile b/runtime/druntime/test/shared/Makefile
index 0ba78e6ff9..1c40a7dd91 100644
--- a/runtime/druntime/test/shared/Makefile
+++ b/runtime/druntime/test/shared/Makefile
@@ -139,16 +139,16 @@ else
 endif
 
 $(ROOT)/linkD$(DOTEXE): $(SRC)/linkD.c $(ROOT)/lib$(DOTDLL) $(DRUNTIMESO)
-	$(QUIET)$(CC) $(CFLAGS) $(CC_OUTFLAG)$@ $(if $(findstring $(OS),windows),/[email protected],) $< $(ROOT)/lib$(DOTIMPLIB) $(CC_EXTRAS)
+	$(QUIET)$(CC) $(CFLAGS) $(CC_OUTFLAG)$@ $(if $(findstring $(OS),windows),/Fo$(ROOT)/,) $< $(ROOT)/lib$(DOTIMPLIB) $(CC_EXTRAS)
 
 $(ROOT)/linkDR$(DOTEXE): $(SRC)/linkDR.c $(ROOT)/lib$(DOTDLL) $(DRUNTIMESO)
-	$(QUIET)$(CC) $(CFLAGS) $(CC_OUTFLAG)$@ $(if $(findstring $(OS),windows),/[email protected],) $< $(DRUNTIME_IMPLIB) $(CC_EXTRAS)
+	$(QUIET)$(CC) $(CFLAGS) $(CC_OUTFLAG)$@ $(if $(findstring $(OS),windows),/Fo$(ROOT)/,) $< $(DRUNTIME_IMPLIB) $(CC_EXTRAS)
 
 $(ROOT)/loadDR$(DOTEXE): $(SRC)/loadDR.c $(ROOT)/lib$(DOTDLL) $(DRUNTIMESO)
-	$(QUIET)$(CC) $(CFLAGS) $(CC_OUTFLAG)$@ $(if $(findstring $(OS),windows),/[email protected],) $< $(CC_EXTRAS)
+	$(QUIET)$(CC) $(CFLAGS) $(CC_OUTFLAG)$@ $(if $(findstring $(OS),windows),/Fo$(ROOT)/,) $< $(CC_EXTRAS)
 
 $(ROOT)/host$(DOTEXE): $(SRC)/host.c $(ROOT)/plugin1$(DOTDLL) $(ROOT)/plugin2$(DOTDLL)
-	$(QUIET)$(CC) $(CFLAGS) $(CC_OUTFLAG)$@ $(if $(findstring $(OS),windows),/[email protected],) $< $(CC_EXTRAS)
+	$(QUIET)$(CC) $(CFLAGS) $(CC_OUTFLAG)$@ $(if $(findstring $(OS),windows),/Fo$(ROOT)/,) $< $(CC_EXTRAS)
 
 $(ROOT)/liblinkdep$(DOTDLL): $(ROOT)/lib$(DOTDLL)
 $(ROOT)/liblinkdep$(DOTDLL): DFLAGS+=-L$(ROOT)/lib$(DOTIMPLIB)

the-horo · 2024-09-13T14:33:21Z

If we can get the tests to pass I think these changes are pretty much everything that we need and that we can do simply (we're only ignoring RT_LFLAGS). I still want to rewrite the makefiles but that's between me and dmd upstream. If I end up doing it maybe I'll change some of the variable names but the current way of passing flags and CC should still work the same.

kinke · 2024-09-13T15:21:38Z

The windows tests are also failing:

C:/Program Files/LLVM/bin/clang-cl.exe /Wall /O2 /FeD:/a/ldc/build/runtime/druntime-test-shared-release/linkD.exe /FoD:/a/ldc/build/runtime/druntime-test-shared-release/linkD.exe.obj src/linkD.c D:/a/ldc/build/runtime/druntime-test-shared-release/lib.lib 
clang-cl: error: cannot specify '/FoD:/a/ldc/build/runtime/druntime-test-shared-release/linkD.exe.obj' when compiling multiple source files

Sigh, this seems like a silly clang-cl bug - yeah, we have one C source file and one library as inputs, but common, you're only compiling a single source file, so please accept the desired object file path, as Microsoft's cl.exe does. - Undetected in CI so far because we switch to clang-cl for the CMake config in

ldc/.github/actions/helper-build-ldc/action.yml

Lines 58 to 59 in 1f0c36e

    
                     -DCMAKE_C_COMPILER=clang-cl ^ 
        
                     -DCMAKE_CXX_COMPILER=clang-cl ^

but don't have any CC env variable set at test-time.

Thanks for looking into a workaround. Unfortunately this is in upstream...

…ation tests]

kinke · 2024-09-13T18:02:59Z

.github/workflows/main.yml

            extra_cmake_flags: >-
              -DBUILD_LTO_LIBS=ON
+              -DRT_CFLAGS=-m32


Heh, this is pretty similar to the Gentoo situation: the CFLAGS env variable used to set up 64-bit clang-cl as C compiler only applies at CMake config-time, affecting the CMake default flags and making it to the builds:

ldc/.github/actions/1-setup/action.yml

Lines 165 to 172 in 1f0c36e

- name: 'Windows x86: Make CMake configure 64-bit clang-cl for 32-bit code emission'

if: runner.os == 'Windows' && inputs.arch == 'x86'

shell: bash

run: |

set -eux

echo "CFLAGS=-m32" >> $GITHUB_ENV

echo "CXXFLAGS=-m32" >> $GITHUB_ENV

echo "ASMFLAGS=-m32" >> $GITHUB_ENV

But now with clang-cl as druntime integration test CC, that -m32 isn't forwarded, and we need to pass it in via RT_CFLAGS explicitly.

[This worked fine with default Microsoft cl C compiler from the environment (PATH), which diverges (with different executables in different dirs) across x64 and x86. clang-cl doesn't infer the bitness from the environment apparently.]

the-horo and others added 5 commits September 11, 2024 11:37

runtime/CMakeLists.txt: Call cmake_minimum_required before project()

ee381a3

Signed-off-by: Andrei Horodniceanu <[email protected]>

runtime/CMakeLists.txt: add call to enable_testing()

dcf8555

Signed-off-by: Andrei Horodniceanu <[email protected]>

runtime/DRuntimeIntegrationTests.cmake: Pass D_EXTRA_FLAGS to ldmd

0dcf778

D_EXTRA_FLAGS is the intended way to pass flags like -m32 to ldc so make sure that its value is respected during the druntime tests. Signed-off-by: Andrei Horodniceanu <[email protected]>

[druntime: Pass D_EXTRA_FLAGS and RT_CFLAGS in {C,D}FLAGS_BASE for ma…

7f31303

…ke-based integration tests]

kinke mentioned this pull request Sep 12, 2024

Better support building the runtime separately #4743

Merged

[druntime: Pass CMAKE_C[XX]_COMPILER as CC/CXX for make-based integra…

48b38b2

…tion tests] To propagate the C[++] compilers at CMake configure-time to test-time.

kinke commented Sep 13, 2024

View reviewed changes

[ldc-build-runtime: Define LDMD_EXE_FULL for druntime integration tests]

71a6f26

[druntime: Work around clang-cl bug in test/shared/Makefile]

074c388

kinke force-pushed the standalone-runtime2 branch from bc1cfe5 to 074c388 Compare September 13, 2024 15:39

kinke added 3 commits September 13, 2024 18:03

CI: Try a different workaround for Apple issue ldc-developers#3901

03abcf8

[tiny refactoring wrt. CXX language dependency for runtime build]

29a1eb5

[fix Win32 CI via explicit -m32 for clang-cl used for druntime integr…

140cb6d

…ation tests]

kinke commented Sep 13, 2024

View reviewed changes

[druntime: Streamline Windows part of stdcpp integration test]

36d6f58

kinke marked this pull request as ready for review September 13, 2024 20:14

kinke added 3 commits September 20, 2024 11:18

[CI: Make workaround for ldc-developers#3901 a bit less ugly]

cd9f493

[druntime: Clarify that DFLAGS_BASE in common.mak are an LDC extension]

99883ac

GHA: Switch to Xcode 16 final

4d908d3

kinke merged commit 8a30f4d into ldc-developers:master Sep 20, 2024
20 checks passed

kinke deleted the standalone-runtime2 branch September 20, 2024 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better support building the runtime separately [mod] #4752

Better support building the runtime separately [mod] #4752

kinke commented Sep 12, 2024

the-horo commented Sep 12, 2024

kinke commented Sep 13, 2024 •

edited

Loading

kinke Sep 13, 2024 •

edited

Loading

the-horo Sep 13, 2024

kinke commented Sep 13, 2024

the-horo commented Sep 13, 2024

the-horo commented Sep 13, 2024

kinke commented Sep 13, 2024 •

edited

Loading

kinke Sep 13, 2024

		cmake_minimum_required(VERSION 3.4.3)
		project(runtime C ASM CXX) # CXX for integration tests only

	- name: 'Windows x86: Make CMake configure 64-bit clang-cl for 32-bit code emission'
	if: runner.os == 'Windows' && inputs.arch == 'x86'
	shell: bash
	run: \|
	set -eux
	echo "CFLAGS=-m32" >> $GITHUB_ENV
	echo "CXXFLAGS=-m32" >> $GITHUB_ENV
	echo "ASMFLAGS=-m32" >> $GITHUB_ENV

Better support building the runtime separately [mod] #4752

Better support building the runtime separately [mod] #4752

Conversation

kinke commented Sep 12, 2024

the-horo commented Sep 12, 2024

1. respect configure-time CC and CXX

2. (why we can't) use CC to pass arguments

3. divergence of the makefiles from the standard way of passing flags

What I propose as a fix

kinke commented Sep 13, 2024 • edited Loading

kinke Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

the-horo Sep 13, 2024

Choose a reason for hiding this comment

kinke commented Sep 13, 2024

the-horo commented Sep 13, 2024

the-horo commented Sep 13, 2024

kinke commented Sep 13, 2024 • edited Loading

kinke Sep 13, 2024

Choose a reason for hiding this comment

kinke commented Sep 13, 2024 •

edited

Loading

kinke Sep 13, 2024 •

edited

Loading

kinke commented Sep 13, 2024 •

edited

Loading