-
-
Notifications
You must be signed in to change notification settings - Fork 268
Support ldc2.conf as a directory #4954
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
Conversation
driver/configfile.cpp
Outdated
|
||
llvm::sort(configFiles, [](const std::string &a, const std::string &b) { | ||
auto sa = llvm::StringRef(a), sb = llvm::StringRef(b); | ||
return sa.compare_numeric(sb) < 0; |
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.
Thx for the (lot of) work! - This was the 1st thing I was curious about, how to define the order. Such a 'smart' sort with a default number prefix sounds good to me. 👍
The current CI errors suggest an incomplete config for the built (non-installed) compiler. Let me know if you need help. |
Yup. I didn't think of testing that so I guess I did forget about something... It did work when installing so surely it can't be that broken. |
runtime/CMakeLists.txt
Outdated
if(PHOBOS2_DIR) | ||
set(build_defaultlib "phobos2-ldc,druntime-ldc") | ||
else() | ||
set(build_defaultlib "druntime-ldc") | ||
endif() |
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.
Is there any situation in which PHOBOS2_DIR
doesn't exist? To fix the android failures I would need to slightly adjust this and I want to know if I can simplify the code a little bit?
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 guess we can drop that special case of a Phobos-less build. I guess the most likely scenario is that someone forgot to initialize the git submodule before running CMake, not intentionally excluding Phobos from the build.
Are those cmake quotes interfering with my command's quotes or is there some other reason it's failing? |
Not sure, I'd need to check locally. In general, using the Edit: I guess the error is earlier, when trying to invoke a .sh script to finalize the |
FYI: instead of many separate small test files, you can use |
Ohh, that looks useful |
You're right:
It seems that cmake is content to let the |
14e8a43
to
f1f7373
Compare
Ok, one test failure for tomorrow me to go through. Other things that I thought of that could be useful:
|
Ah yeah that'd be good.
Hmm; I think the subset of people running tests is way smaller than those just building. And for those druntime integration tests, we set PATH using a hardcoded assumption: Lines 60 to 66 in 3acd761
I still need to check your rationale for switching to explicit triples for the default target. The main problem I saw back then is that there's no clear 1:1 mapping to the normalized triple (the one with the vendor) - e.g. the vendor seems uninteresting for all but Apple targets (and can be anything from empty to Edit: Another example: the (libc) environment. IIRC, one can use the prebuilt Alpine package (targeting |
I don't agree with this point because:
I couldn't find an example of functions having different signatures but for different behavior just pick something from https://wiki.musl-libc.org/functional-differences-from-glibc.html. Here's an example with // b.c
#include <libgen.h>
#include <string.h>
int always_true(void) {
char arr[] = "/lib/foo/";
char *result = basename(arr);
return strcmp(result, "foo") == 0;
} // a.c
#include <unistd.h>
#include <stdio.h>
extern int always_true(void);
int main (int argc, char **argv) {
printf("1 == %d\n", always_true());
} If you compile both of those files on either musl or glibc you get the correct result:
If you compile
With the comment that this behavior in not specified so it can change at anytime (and even break). For this reason, at least when it comes to the cenv part of the triple, it is not safe to normalize it away. I have to think about the version components of the OS and ARCH fields and see if I can draw a conclusion, otherwise
I'll go with your suggestion here. It's not that important to encode the triple, it only makes the code a little shorter and marginally helps |
Yeah I didn't mean to encourage anything like that, just saying that some guys might already use such stuff, regardless of whether by accident or knowing what they are doing. The env can be versioned too - |
Ok, I added back the documentation (though perhaps a little bit too much for a config file) and made the cat portable on windows. Still unsure what to do with the default triple situation, it sounds most reasonable to skip the last commit and keep |
Changed the documentation file to only be installed with the compiler, not with every runtime build. |
3447df9
to
b15cbe9
Compare
Tests passed but I'm looking over the artifacts and checking that the config files aren't broken. The android one seems to be so I'll have to fix it |
3d82ec7
to
3c35ebb
Compare
The android config now looks fine and I've changed it to be a directory in the release artifacts. The test failure is a timeout |
runtime/ldc-build-runtime.d.in
Outdated
@@ -159,7 +160,6 @@ void runCMake() { | |||
"-DLDMD_EXE_FULL=" ~ ldmdExecutable, | |||
"-DDMDFE_MINOR_VERSION=@DMDFE_MINOR_VERSION@", | |||
"-DDMDFE_PATCH_VERSION=@DMDFE_PATCH_VERSION@", | |||
"-DLDC_WITH_LLD=@LDC_WITH_LLD@", | |||
"-DINCLUDE_INSTALL_DIR=@INCLUDE_INSTALL_DIR@", |
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 think we don't need this anymore.
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.
yeah, and INCLUDE_INSTALL_DIR
can be taken out of runtime/CMakeLists.txt
since only the compiler uses it.
runtime/ldc-build-runtime.d.in
Outdated
@@ -3,6 +3,7 @@ module ldcBuildRuntime; | |||
import core.stdc.stdlib : exit; | |||
import std.algorithm; | |||
import std.array; | |||
import std.conv; |
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.
[nit: now superfluous]
69cebae
to
d2939e4
Compare
If everything works, possible followups include:
|
cf4c46c
to
6a4223a
Compare
See-Also: ldc-developers/ldc#4954 Signed-off-by: Andrei Horodniceanu <[email protected]>
Okay, this was my final review pass now, just a few nits remaining. I wanna follow-up on #4954 (comment) after this, shuffling around the order and filenames a bit. |
If ldc2.conf is a directory go through each file (directly) inside it, ordered by name and apply the settings in it. It would be similar to appending all the files and treating it like a normal config file. The specific ordering is the one from llvm::StringRef::compare_numeric. The previous behavior was to always error out if ldc2.conf was a directory, being it passed through `-conf` or being automatically picked up. To account for this change the following restrictions that affect config files have been lifted: - at least one section must match the current triple This is undesirable for cross-compile setups in which one config file is dedicated to one triple - both switches and post-switches must be defined This is undesirable for config files that simply append a CLI switch The above have been replaced by a check that all settings represent valid keys (so switches, post-switches etc.) and that they are of the correct type. Signed-off-by: Andrei Horodniceanu <[email protected]>
When building, always generate separate files for each ldc2.conf section. This allows creating a full ldc2.conf, part by part, without carrying about the statements' order in the cmake file. When installing, `cat` all the configured files and install it as a single ldc2.conf file. Added the CONF_PREF_DIR cmake option for installing the conf files as a directory, without concatenating them. This is useful when building the runtime as a separate project as this setting would allow generating a (partial) ldc2.conf, enough to support the runtime libraries, which could latter be merged with the config generated by the root ldc project to get a fully functional ldc2. This is a requirement when cross-compiling and installing the libraries on another device or when performing multilib builds outside of -DMULTILIB=ON Also add a more detailed documentation about the configuration. Signed-off-by: Andrei Horodniceanu <[email protected]>
Some settings like the default wasm switches or the compiler-rt lib-dir don't have anything to do with the runtime and, on top of that, some are conditioned by build settings that only affect the compiler. Moving them out of the runtime directory improves the support for cross-compiling because, in the scenario, some variables may be unset in the runtime directory as there is no longer a higher-level project to set them. Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Alright, thx a lot for the hard work, cool stuff! 👍 |
Short summary of changes:
=> easier to configure independent sections of settings
Compiler contains the
-I
includes, wasm flags and compiler-rt libdirRuntime contains phobos&druntime libdir and rpath
=> The runtime and the compiler can now be installed as separate projects without their config files colliding/being skipped
x86_64-.*-linux-gnu
instead of"default"
).=>
ldc-build-runtime
can now install cross-builds alongside their configuration files (it skipped the config previously to avoid overwriting the host one), removing the need for post-install interaction from the user.