Skip to content
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

fix 2698: cImportPaths doesn't work with dmd and ldc #2818

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

apbryan
Copy link

@apbryan apbryan commented Jan 19, 2024

Addresses issue 2698 where the wrong flags are passed to dmd and ldc2 such that cImportPaths do not work. I tested with the dmd version I have currently installed (2.106.1), but have not tested with the old version of ldc2 I have (1.28.0 based on dmd 2.098.0) as it does not appear to support the -P arg for passing C preprocessor flags

Still need to:

  • Test with newer version of ldc
  • Create changelog file

Open question: should dub attempt to detect if the version of the compiler supports cImportPaths ?

Copy link

github-actions bot commented Jan 19, 2024

✅ PR OK, no changes in deprecations or warnings

Total deprecations: 8

Total warnings: 0

Build statistics:

 statistics (-before, +after)
 executable size=5293368 bin/dub
-rough build time=62s
+rough build time=61s
Full build output
DUB version 1.38.0, built on Jul  4 2024
LDC - the LLVM D compiler (1.39.0):
  based on DMD v2.109.1 and LLVM 18.1.6
  built with LDC - the LLVM D compiler (1.39.0)
  Default target: x86_64-unknown-linux-gnu
  Host CPU: znver3
  http://dlang.org - http://wiki.dlang.org/LDC


  Registered Targets:
    aarch64     - AArch64 (little endian)
    aarch64_32  - AArch64 (little endian ILP32)
    aarch64_be  - AArch64 (big endian)
    amdgcn      - AMD GCN GPUs
    arm         - ARM
    arm64       - ARM64 (little endian)
    arm64_32    - ARM64 (little endian ILP32)
    armeb       - ARM (big endian)
    avr         - Atmel AVR Microcontroller
    bpf         - BPF (host endian)
    bpfeb       - BPF (big endian)
    bpfel       - BPF (little endian)
    hexagon     - Hexagon
    lanai       - Lanai
    loongarch32 - 32-bit LoongArch
    loongarch64 - 64-bit LoongArch
    mips        - MIPS (32-bit big endian)
    mips64      - MIPS (64-bit big endian)
    mips64el    - MIPS (64-bit little endian)
    mipsel      - MIPS (32-bit little endian)
    msp430      - MSP430 [experimental]
    nvptx       - NVIDIA PTX 32-bit
    nvptx64     - NVIDIA PTX 64-bit
    ppc32       - PowerPC 32
    ppc32le     - PowerPC 32 LE
    ppc64       - PowerPC 64
    ppc64le     - PowerPC 64 LE
    r600        - AMD GPUs HD2XXX-HD6XXX
    riscv32     - 32-bit RISC-V
    riscv64     - 64-bit RISC-V
    sparc       - Sparc
    sparcel     - Sparc LE
    sparcv9     - Sparc V9
    spirv       - SPIR-V Logical
    spirv32     - SPIR-V 32-bit
    spirv64     - SPIR-V 64-bit
    systemz     - SystemZ
    thumb       - Thumb
    thumbeb     - Thumb (big endian)
    ve          - VE
    wasm32      - WebAssembly 32-bit
    wasm64      - WebAssembly 64-bit
    x86         - 32-bit X86: Pentium-Pro and above
    x86-64      - 64-bit X86: EM64T and AMD64
    xcore       - XCore
   Upgrading project in /home/runner/work/dub/dub/
    Starting Performing "release" build using /opt/hostedtoolcache/dc/ldc2-1.39.0/x64/ldc2-1.39.0-linux-x86_64/bin/ldc2 for x86_64.
    Building dub 1.39.0-beta.1+commit.7.g8d1b2a37: building configuration [application]
source/dub/internal/dyaml/composer.d(210,43): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/composer.d(232,43): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/composer.d(336,43): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(196,5): Deprecation: cannot access overlapped field `Event.explicitDocument` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(214,5): Deprecation: cannot access overlapped field `Event.explicitDocument` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(241,5): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(148,5): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(148,5): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
     Linking dub
STAT:statistics (-before, +after)
STAT:executable size=5293368 bin/dub
STAT:rough build time=61s

Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would merge this because it is obviously correct, but do you think we could add a test ?

@Geod24
Copy link
Member

Geod24 commented Jan 19, 2024

Open question: should dub attempt to detect if the version of the compiler supports cImportPaths ?

Everything is in place for it to work, so it could do it.

@apbryan
Copy link
Author

apbryan commented Jan 19, 2024

Tested (on my machine) with ldc2 1.35.0 based on DMD v2.105.2

Moving on to adding a test case to $REPO/test. Will the automatic test runner be running with a new enough version of ldc?

@Geod24
Copy link
Member

Geod24 commented Jan 22, 2024

Looks like the test is failing ?

@apbryan
Copy link
Author

apbryan commented Jan 22, 2024

Looks like the test is failing ?

The test script attempts a build of a dummy project first with dmd, then with ldc via dub's --compiler flag. The jobs labeled with dmd fail at the ldc step, and the jobs labeled with ldc fail at the dmd step. Do the jobs labeled with a certain compiler not have any of the other compilers installed?

@Geod24
Copy link
Member

Geod24 commented Jan 22, 2024

Do the jobs labeled with a certain compiler not have any of the other compilers installed?

That's correct, to prevent those kinds of mistake.

@apbryan
Copy link
Author

apbryan commented Jan 22, 2024

Do the jobs labeled with a certain compiler not have any of the other compilers installed?

That's correct, to prevent those kinds of mistake.

So if I change test/issue2698-cimport-paths-broken-with-dmd-ldc.sh to simply call ${DUB} build --root test/issue2698-cimoort-paths-broken-with-dmd-ldc, is this redundant? I noticed there's a test/run-unittest.sh that will go through and run dub build/run/test on every folder in test/ that doesn't have a special dotfile to disable build/run/test. Should I delete the issue-specific .sh file (which only call dub build) and depend on run-unittest.sh

@Geod24
Copy link
Member

Geod24 commented Jan 23, 2024

Yes, if you have have something that just succeeds/fails with dub build it's less work.

@apbryan
Copy link
Author

apbryan commented Jan 28, 2024

Ok, so the only builds that are failing at the moment are windows builds with dmd. Here is the relevant output:

[INFO] Building /d/a/dub/dub/test/issue2698-cimportpaths-broken-with-dmd-ldc/...
    Starting Performing "debug" build using dmd for x86_64.
    Building issue2698-cimportpaths-broken-with-dmd-ldc ~master: building configuration [application]
Error: C preprocess command C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\cl.exe failed for file test\issue2698-cimportpaths-broken-with-dmd-ldc\source\foo.c, exit status 2

C:\hostedtoolcache\windows\dc\dmd-2.106.1\x64\dmd2\windows\bin64\..\..\src\druntime\import\importc.h(134): fatal error C1083: Cannot open include file: 'sal.h': No such file or directory
Error dmd failed with exit code 1.
[ERROR] Build failure.
[INFO] Running /d/a/dub/dub/test/issue2698-cimportpaths-broken-with-dmd-ldc/...
    Starting Performing "debug" build using dmd for x86_64.
    Building issue2698-cimportpaths-broken-with-dmd-ldc ~master: building configuration [application]
Error: C preprocess command C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\cl.exe failed for file test\issue2698-cimportpaths-broken-with-dmd-ldc\source\foo.c, exit status 2

C:\hostedtoolcache\windows\dc\dmd-2.106.1\x64\dmd2\windows\bin64\..\..\src\druntime\import\importc.h(134): fatal error C1083: Cannot open include file: 'sal.h': No such file or directory
Error dmd failed with exit code 1.
[ERROR] Run failure.
[INFO] Testing /d/a/dub/dub/test/issue2698-cimportpaths-broken-with-dmd-ldc/...
             Generating test runner configuration 'issue2698-cimportpaths-broken-with-dmd-ldc-test-library' for 'library' (library).
    Starting Performing "unittest" build using dmd for x86_64.
    Building issue2698-cimportpaths-broken-with-dmd-ldc ~master: building configuration [issue2698-cimportpaths-broken-with-dmd-ldc-test-library]
Error: C preprocess command C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\cl.exe failed for file source\foo.c, exit status 2

C:\hostedtoolcache\windows\dc\dmd-2.106.1\x64\dmd2\windows\bin64\..\..\src\druntime\import\importc.h(134): fatal error C1083: Cannot open include file: 'sal.h': No such file or directory
Error dmd failed with exit code 1.
[ERROR] Test failure.

I hate to point fingers......but it looks to me like this error is unrelated to my changes. It'll take me a little time to get a dev environment set up on windows to look into this. In the meantime, do you (@Geod24) see anything obvious to point me in the right direction?

@takinutani
Copy link

takinutani commented Feb 3, 2024

@apbryan
ImportC does not work with dmd on Windows as it is.
Issue 24308 - [ImportC] druntime\import\importc.h(134): fatal error C1034: sal.h: no include path set
Issue 24111 - [ImportC] fatal error C1034: stdio.h: no include path set

D:\labo\dlang\dub\test\issue2698-cimportpaths-broken-with-dmd-ldc>set VCTOOLSINSTALLDIR
Environment variable VCTOOLSINSTALLDIR not defined

D:\labo\dlang\dub\test\issue2698-cimportpaths-broken-with-dmd-ldc>set VCINSTALLDIR
Environment variable VCINSTALLDIR not defined

D:\labo\dlang\dub\test\issue2698-cimportpaths-broken-with-dmd-ldc>set UniversalCRTSdkDir
Environment variable UniversalCRTSdkDir not defined

D:\labo\dlang\dub\test\issue2698-cimportpaths-broken-with-dmd-ldc>set WindowsSdkDir
Environment variable WindowsSdkDir not defined

D:\labo\dlang\dub\test\issue2698-cimportpaths-broken-with-dmd-ldc>..\..\bin\dub run --compiler=dmd
    Starting Performing "debug" build using dmd for x86_64.
    Building issue2698-cimportpaths-broken-with-dmd-ldc ~master: building configuration [application]
cl : コマンド ライン warning D9002 : 不明なオプション '/Zc:preprocessor' を無視します。
cl : コマンド ライン warning D9002 : 不明なオプション '/PD' を無視します。
D:\scoop\user\apps\dmd\current\windows\bin64\..\..\src\druntime\import\importc.h(140): fatal error C1083: include t@CJ܂B'sal.h':No such file or directory
Error: C preprocess command D:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\bin\HostX64\x64\cl.exe failed for file source\foo.c, exit status 2

Error dmd failed with exit code 1.

In order for ImportC to work, Visual Studio(or Build Tools) and Windows SDK must be installed and the Visual Studio environment variables must be set.
Startup VS Native Tools Command Prompt and run.

d:\labo\dlang\dub\test\issue2698-cimportpaths-broken-with-dmd-ldc>..\..\bin\dub run --compiler=dmd
    Starting Performing "debug" build using dmd for x86_64.
  Up-to-date issue2698-cimportpaths-broken-with-dmd-ldc ~master: target for configuration [application] is up to date.
    Finished To force a rebuild of up-to-date targets, run again with --force
     Running issue2698-cimportpaths-broken-with-dmd-ldc.exe
func bar in foo.h

Unlike LDC, the environment variable setting process in dmd does not seem to be working properly yet.

D:\labo\dlang\dub\test\issue2698-cimportpaths-broken-with-dmd-ldc>dmd --version
DMD64 D Compiler v2.107.0

@Geod24
Copy link
Member

Geod24 commented Feb 6, 2024

@apbryan : I took the liberty to rebase your PR and try a solution to the problem. That didn't quite work, I'll try a few more things tonight.

@apbryan
Copy link
Author

apbryan commented Feb 6, 2024

@apbryan : I took the liberty to rebase your PR and try a solution to the problem. That didn't quite work, I'll try a few more things tonight.

Wonderful. I haven't had time to dig into this myself (doing very important work playing stormgate). It's nice to see the work being done!

@takinutani
Copy link

@Geod24
I tried Github Actions.
I only needed to set up the preferences for DMD, so I was able to pass with the following settings.

- name: '[Windows] Add VC toolset to PATH (DMD only)'
      if: ${{ runner.os == 'Windows' && env.DC == 'dmd' }}
      uses: ilammy/msvc-dev-cmd@v1

@shoo
Copy link
Contributor

shoo commented May 10, 2024

@apbryan @Geod24
That windows-dmd error is due to https://issues.dlang.org/show_bug.cgi?id=24111
The bug fixes I have implemented will fix this.
I would recommend waiting for the current dmd-master to become dmd-latest ( release dmd 2.109.0 ), then trying git rebase to dub's master.
Or, as @takinutani points out, setting the INCLUDE environment variable in msvc will solve the problem.

Copy link
Contributor

@shoo shoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, I will point out my review.

Comment on lines +3 to +6
int bar(void)
{
printf("func bar in foo.h\n");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing the implementation in .h files is a terrible practice.
How about declaring the type as a struct or something instead of a function?

@shoo
Copy link
Contributor

shoo commented Jun 10, 2024

I think this PR would pass the test now.

@apbryan
Copy link
Author

apbryan commented Aug 4, 2024

Looks like one automated test failed (buildkite/dub) because whatever box/VM it was running on didn't have a new enough version of LLVM to build ldc with.

From the log:
CMake Error at cmake/Modules/FindLLVM.cmake:62 (message):
Unsupported LLVM version 14.0.0 found (/bin/llvm-config). At least version
15.0 is required. You can also set variables 'LLVM_ROOT_DIR' or
'LLVM_CONFIG' to use a different LLVM installation.
Call Stack (most recent call first):
cmake/Modules/FindLLVM.cmake:190 (_LLVM_FAIL)
CMakeLists.txt:37 (find_package)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants