Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

[STABLE] Add missing binding in SRCS #1818

Merged
merged 1 commit into from May 26, 2017

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Apr 27, 2017

Some files were not part of mak/SRCS, and thus were not compiled it.

Since they are C bindings, it was unnoticed for a long time,
as most operations won't require the rare symbols this module defined.
However for things such as static array declarations,
the __init of the struct might be required, which triggers a linker error.

One simple way to trigger the issue was to use the function timersub defined in
core.sys.linux.sys.time:

src/ocean/core/UnitTestRunner.d:671: undefined reference to 'core.sys.linux.sys.time.timersub(const(core.sys.posix.sys.time.timeval*), const(core.sys.posix.sys.time.timeval*), core.sys.posix.sys.time.timeval*)'

The patch is a superset of: https://github.com/sociomantic-tsunami/ocean/blob/fef725ca9643af4ffcccb44bcc8a2ff7598c4e24/docker/dmd-transitional/patches/druntime/0010-Add-missing-binding-in-SRCS.patch

@schveiguy
Copy link
Member

LGTM

@mathias-lang-sociomantic
Copy link
Contributor

Win32 error: Error: command line too long

Great. I guess it's time to use this little DMD feature.

@Geod24 Geod24 force-pushed the missing-bindings branch 2 times, most recently from 016077b to b805dc0 Compare April 29, 2017 12:59
@Geod24
Copy link
Member Author

Geod24 commented Apr 29, 2017

Didn't seem to help. @braddr , am I correct that Win_32_64 is using DigitalMars' Make ? Could you offer some advice on this issue (Command too long) ?

@rainers
Copy link
Member

rainers commented Apr 29, 2017

That's likely a limitation of the Digital Mars make: the win64 build command line is slightly above 10000 characters, while the win32 is just a couple of characters below. It used to be 8192 in the command line interpreter, but it's 32768 for the CreateProcess function (https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx).

The usual workaround is to place the command line arguments into a temporary file and use @tmpfile on the command line.

@Geod24
Copy link
Member Author

Geod24 commented Apr 29, 2017

The usual workaround is to place the command line arguments into a temporary file and use @tmpfile on the command line.

According to the manual that's what using * and ~ should do.

@rainers
Copy link
Member

rainers commented Apr 29, 2017

According to the manual that's what using * and ~ should do.

Ah, I wasn't aware of this functionlity. It actually seems to work here until a higher length (probably 32768 as the limit of environment strings).

@mathias-lang-sociomantic
Copy link
Contributor

CC @WalterBright : I tried using * and ~ inside the Win32 Makefile so DigitialMars' MAKE would pass the command line args via @_CMDLINE, but it doesn't seem to have any effect.

@schveiguy
Copy link
Member

If the issue here is windows make, can't we do something different for it? Clearly windows doesn't need linux headers.

@CyberShadow
Copy link
Member

According to the manual that's what using * and ~ should do.

No, that's something else. Those cause DigitalMars Make to pass the command line in an environment variable, not a file. This is functionality specific to the DigitalMars toolchain, and although it is apparently supported by OPTLINK, it is not supported by dmd itself.

I suggest trying manually write the list to a temporary file in the Makefile target rule.

@rainers
Copy link
Member

rainers commented May 13, 2017

it is not supported by dmd itself.

Unfortunately, it does support it: https://github.com/dlang/dmd/blob/master/src/ddmd/root/response.d#L65

Nevertheless, I'd also suggest to use an explicit temporary file, too.

@mathias-lang-sociomantic
Copy link
Contributor

We had a discussion with @WalterBright at DConf about it. He offered to give me access to DM Make to track the problem down. I'd rather fix the issue than add another workaround.

@rainers
Copy link
Member

rainers commented May 20, 2017

The trouble is just that you have applied the "~" fix to win32.mak, but not to win64.mak.

@Geod24 Geod24 force-pushed the missing-bindings branch 2 times, most recently from 2b9a99d to 96c327e Compare May 20, 2017 17:49
Some files were not part of `mak/SRCS`, and thus were not compiled it.

Since they are C bindings, it was unnoticed for a long time,
as most operations won't require the rare symbols this module defined.
However for things such as static array declarations,
the `__init` of the struct might be required, which triggers a linker error.

One simple way to trigger the issue was to use the function `timersub` defined in
`core.sys.linux.sys.time`:

---
src/ocean/core/UnitTestRunner.d:671: undefined reference to 'core.sys.linux.sys.time.timersub(const(core.sys.posix.sys.time.timeval*), const(core.sys.posix.sys.time.timeval*), core.sys.posix.sys.time.timeval*)'
---

The change to win{32,64}.mak was mandatory as the toolchain on the auto-tester cannot cope with that
number of files, and errors with "Command too long". DMD support 'response file' for that purpose.
Using `*` is a DigitalMars MAKE extension which will pass commands > 996 chars via the env.
@Geod24
Copy link
Member Author

Geod24 commented May 20, 2017

The trouble is just that you have applied the "~" fix to win32.mak, but not to win64.mak.

Oh damn, it was that simple, thanks !
Fixed, but Jenkins is failing with a seemingly unrelated error:

�[0;33m[INFO] Running /var/lib/jenkins/dlang_projects@3/dlang/dub/test/interactive-remove.sh...�[0m
Fetching dub 0.9.20...
Fetching dub 0.9.21...
Cannot remove package 'dub', there are multiple possibilities at location
'user'.
Available versions:
  0.9.20
  ~master
  1.1.0
Please specify a individual version using --version=... or use the wildcard --version=* to remove all versions.
�[0;31mScript failure.�[0m
readlink -f ${BASH_SOURCE[0]}

@wilzbach
Copy link
Member

@Geod24: Jenkins is Linux only (ATM) and unfortunately broken since a couple of days. It's not related to this PR.

@Geod24
Copy link
Member Author

Geod24 commented May 20, 2017

So it won't prevent a merge, despite being Required ?

@wilzbach
Copy link
Member

It does, but @CyberShadow can disable it

@MartinNowak
Copy link
Member

Was a temporary failure with the interactive dub remove test, not exactly sure why it failed. I just reran it, need to change Jenkins auth to allow more people to rerun tests (but am still traveling atm.).

1 similar comment
@MartinNowak
Copy link
Member

Was a temporary failure with the interactive dub remove test, not exactly sure why it failed. I just reran it, need to change Jenkins auth to allow more people to rerun tests (but am still traveling atm.).

@MartinNowak MartinNowak merged commit cc99c09 into dlang:stable May 26, 2017
@Geod24 Geod24 deleted the missing-bindings branch June 1, 2017 20:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants