ldc-developers / ldc2.snap

Snap package definition for LDC, the LLVM-based D compiler
11 stars 4 forks source link

Update ldc2.conf to better match upstream 1.18.0 binary package #104

Closed WebDrake closed 4 years ago

WebDrake commented 4 years ago

This brings the config file in line with upstream standards. It should be backwards compatible, while adding support for some extra settings, such as WebAssembly builds.

The wasm settings have one small difference from upstream: it uses --link-internally instead of -link-internally, on the assumption that the latter is a typo (the flag reported by ldc2 --help starts with --).

The precise diff with upstream:

23a24
> 
26c27,28
<         "-I%%ldcbinarypath%%/../import",
---
>         "-I%%ldcbinarypath%%/../include/d/ldc",
>         "-I%%ldcbinarypath%%/../include/d",
27a30
> 
30,31c33,34
<         "%%ldcbinarypath%%/../lib",
<         "%%ldcbinarypath%%/../lib32",
---
>         "-L-L%%ldcbinarypath%%/../lib64",
>         "-L-L%%ldcbinarypath%%/../lib32",
32a36
> 
34c38
<     rpath = "%%ldcbinarypath%%/../lib:%%ldcbinarypath%%/../lib32";
---
>     rpath = "%%ldcbinarypath%%/../lib64:%%ldcbinarypath%%/../lib32";
41c45
<         "-link-internally",
---
>         "--link-internally",
JohanEngelen commented 4 years ago

one or two hyphens doesn't matter AFAIK, in which case it'd be better to keep it identical to upstream.

WebDrake commented 4 years ago

I'll check and tweak. In any case there's something wrong with this config: the linker is failing to find libs. I'll come back to this in the next day or two ;-)

WebDrake commented 4 years ago

This should fix the linker issues (forgot to drop the -L-L when copying library paths):

diff --git a/ldc-config/ldc2.conf b/ldc-config/ldc2.conf
index fd134a1..b9d3ff9 100644
--- a/ldc-config/ldc2.conf
+++ b/ldc-config/ldc2.conf
@@ -30,8 +30,8 @@ default:

     // default directories to be searched for libraries when linking
     lib-dirs = [
-        "-L-L%%ldcbinarypath%%/../lib64",
-        "-L-L%%ldcbinarypath%%/../lib32",
+        "%%ldcbinarypath%%/../lib64",
+        "%%ldcbinarypath%%/../lib32",
     ];

     // default rpath when linking against the shared default libs
WebDrake commented 4 years ago

@JohanEngelen I slightly object to using a flag that's different from what's documented, but as you say, it makes no odds (I checked). So I've matched upstream:

diff --git a/ldc-config/ldc2.conf b/ldc-config/ldc2.conf
index b9d3ff9..77f255e 100644
--- a/ldc-config/ldc2.conf
+++ b/ldc-config/ldc2.conf
@@ -42,7 +42,7 @@ default:
 {
     switches = [
         "-defaultlib=",
-        "--link-internally",
+        "-link-internally",
         "-L--export-dynamic",
     ];
     lib-dirs = [];

The commit message has been updated to take this into account, and to explain the remaining divergences from upstream packages.

JohanEngelen commented 4 years ago

the flag reported by ldc2 --help starts with --

On my machine (macOS 10.13), "-link-internally" is shown with only one -.

Edit: LDC 1.12 shows it with one -, but LDC 1.20 shows it with two --. ¯\(ツ)

JohanEngelen commented 4 years ago

I slightly object to using a flag that's different from what's documented

Agreed, so let's fix it here or upstream ;-)

WebDrake commented 4 years ago

Well in light of what you say about Mac vs Linux LDC, let's just leave it as-is. Not worth worrying so long as it works.

kinke commented 4 years ago

[This (one vs. 2 dashes) doesn't have to do with Mac vs. Linux, but with LLVM displaying 2 dashes in the help output starting with v9 (LDC 1.18), incl. -O vs. --O3 etc.]

Why does the snap package have its own config file then? It seems totally superfluous (edit: well, after making it portable via %%ldcbinarypath%% as done for the regular packages) and just prone to future regressions.

WebDrake commented 4 years ago

Why does the snap package have its own config file then? It seems totally superfluous (edit: well, after making it portable via %%ldcbinarypath%% as done for the regular packages) and just prone to future regressions.

I agree it's not optimal, and I'd like to change that.

The problem is that the autogeneration of the contents of ldc2.conf gives invalid paths. By default, it doesn't generate the %%ldcbinarypath%% versions, but uses absolute paths. In the snap package build process this winds up with stuff like:

    // default switches appended after all explicit command-line switches
    post-switches = [
        "-I/include/d/ldc",
        "-I/include/d",
    ];

    // default directories to be searched for libraries when linking
    lib-dirs = [
        "/lib64",
        "/lib32",
    ];

    // default rpath when linking against the shared default libs
    rpath = "/lib64:/lib32";

Way back in the dawn of time I discussed this with LDC devs and the conclusion reached was that it was simpler for the snap package to just maintain its own config file (and keep it up to date) rather than trying to hackily work around the autogenerated file.

If the autogenerated file could be relied on to generate correct %%ldcbinarypath%% specifiers automatically, this wouldn't be an issue. But AFAICS the current LDC build process uses search-and-replace to handle this itself: https://github.com/ldc-developers/ldc/blob/dc2c8546554ec43960d81acdb43e875e0d947fec/.azure-pipelines/posix.yml#L218 https://github.com/ldc-developers/ldc/blob/dc2c8546554ec43960d81acdb43e875e0d947fec/.azure-pipelines/posix.yml#L232

WebDrake commented 4 years ago

I am also not sure that the particular search-and-replace style used in the upstream build process will be reliable inside the snap build.

Have more recent LDC versions started autogenerating files with %%ldcbinarypath%% already in place without search-and-replace?

kinke commented 4 years ago

Nope, that's still the same search-and-replace manual step. What's replaced is the (arbitrary) used CMAKE_INSTALL_PREFIX and INCLUDE_INSTALL_DIR in the CMake cmdline.

WebDrake commented 4 years ago

Which version did that change with? Is it your recent Jan 12 patch (so, the 1.20 release) or further back?

kinke commented 4 years ago

No changes in this regard. What I meant is that you can use an abitrary install dir, replace it in the conf file and, if required for snap, move it to the final location.

WebDrake commented 4 years ago

What I meant is that you can use an abitrary install dir, replace it in the conf file and, if required for snap, move it to the final location.

This is potentially a little tricky inasmuch as snapcraft by default likes to set the cmake install dir itself. It's only a quirk of how the current snapcraft.yaml is set up that there's any chance of manually specifying it.

I'll have a think about how best to handle that in the snap build process. Meanwhile, I'll take this in so we can move forward with newer releases.