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

Add dub test #56

Merged
merged 2 commits into from
May 1, 2021
Merged

Add dub test #56

merged 2 commits into from
May 1, 2021

Conversation

jmh530
Copy link
Contributor

@jmh530 jmh530 commented Jun 16, 2020

I am marking this as a WIP per here. I have not been able to test this on my own yet.

@@ -75,9 +82,9 @@ if grep -q "^--- .*d" "$onlineapp" > /dev/null 2>&1 ; then
exec timeout -s KILL ${TIMEOUT:-30} bash -c "${DLANG_EXEC} -g $args "${d_files[@]:1}" $with_run "${d_files[0]}" ${run_args} | tail -n100000"
fi
elif grep -qE "dub[.](sdl|json):" "$onlineapp" > /dev/null 2>&1 ; then
exec timeout -s KILL ${TIMEOUT:-30} dub -q --compiler=${DLANG_EXEC} --single --skip-registry=all "$onlineapp" ${run_args} | tail -n10000
exec timeout -s KILL ${TIMEOUT:-30} dub ${DUB_COMMAND} -q --compiler=${DLANG_EXEC} --single --skip-registry=all "$onlineapp" ${run_args} | tail -n10000
Copy link
Member

Choose a reason for hiding this comment

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

I don't think test works with --single.

% dub test --single source/scpp/build.d
Package cpp_build (configuration "application") defines no import paths, use {"importPaths": [...]} or the default package directory structure to fix this.
Generating test runner configuration 'cpp_build-test-application' for 'application' (executable).
Source file '/Users/geod24/projects/bpfk/agora/source/scpp/build.d' not found in any import path.

Script used is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that is a dub bug? It is listed as an option under the test docs.

I see some issues in the dub repo about single. It rebuilds every time, it doesn't work for dynamic libraries, and it can fail when the network is bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Geod24 After actually being able to run it myself, I can confirm that I run into that issue.

Using the most recent branch, I built with docker build -t dlangtour/core-exec:dmd . and ran

source="/++dub.sdl: name\"foo\" \n dependency\"mir\" version=\"*\"+/ unittest { import mir.combinatorics, std.stdio; writeln([0, 1].permutations); } void main() {}"
bsource=$(echo -e "$source" | base64 -w0)
DOCKER_FLAGS="-unittest" docker run -e DOCKER_FLAGS --rm dlangtour/core-exec:dmd "$bsource"

and it printed

Package foo (configuration "application") defines no import paths, use {"importPaths": [...]} or the default package directory structure to fix this.
Source file '/sandbox/onlineapp.d' not found in any import path.

When excluding dependencies (code below), I get the same error.

source="/++dub.sdl: name\"foo\" \n +/ unittest { assert(1 > 0); } void main() {}"
bsource=$(echo -e "$source" | base64 -w0)
DOCKER_FLAGS="-unittest" docker run -e DOCKER_FLAGS --rm dlangtour/core-exec:dmd "$bsource"

It's not as if you can remove --single when handling dub test only. That doesn't work. And you need --single when using dub run.

@jmh530
Copy link
Contributor Author

jmh530 commented Mar 28, 2021

@Geod24 This is now passing on dmd after v2.096 fixed --single.

Two questions:

  1. I'm not sure how to do it, but could I warn the user that the current version of the compiler is too low to get this feature to work?
  2. It wasn't compiling when the example had its own main function, as if dub was adding another main function when it shouldn't. I'm not entirely sure why this was happening. This means that if the user provides unittests and a main function, it is probably going to cause an error. Before this is merged, I should include an example that works with both unittests and a main function (potentially with two so that it properly handles calling one or the other), but I'm not quite sure how to fix the issue at the moment.

@jmh530
Copy link
Contributor Author

jmh530 commented Mar 28, 2021

@Geod24 Fixed the first issue I mentioned above. Now passing for ldc2 as well.

@PetarKirov
Copy link
Member

PetarKirov commented Mar 28, 2021

  1. I'm not sure how to do it, but could I warn the user that the current version of the compiler is too low to get this feature to work?

Even though Dub is shipped with the compiler, it really is a separate project (which supports multiple compiler versions), so we shouldn't use the bundled version of Dub, but instead always use the latest Dub version.

I'm not sure, but I think changing this line:

RUN bash /tmp/install.sh -p /dlang install ${DLANG_VERSION}

to:

RUN bash /tmp/install.sh -p /dlang install ${DLANG_VERSION},dub

should do the trick, according to:
https://dlang.org/install

Edit: That said, we really need to fix the GH releases of Dub, as the latest one is 1.23.0 (https://github.com/dlang/dub/releases), and the install script won't be able to install a newer one - see https://github.com/dlang/installer/blob/7b71542a59b6ae095fc867d4e6eb5e8b6a04573a/script/install.sh#L1245.

@jmh530
Copy link
Contributor Author

jmh530 commented Mar 29, 2021

@PetarKirov I have incorporated your suggestion with respect to putting things in terms of dub versions rather than dlang versions. However, the ldc runs are failing. The ldc-beta is giving an error about an undefined reference to main that I don't get on dmd. This is similar to the errors I had before (what drove me to condition it based on dlang version) and also might relate to my prior question about main.

@jmh530
Copy link
Contributor Author

jmh530 commented Mar 29, 2021

To the second point I raised with @Geod24 regarding main, it seems to be an issue with dub inserting an empty main function with dub test --single regardless of whether the function contains a main function or not. I have submitted an issue here.

I'm still not entirely sure how this relates to the ldc error. The only thing I can think of is if a different version of dub is getting installed there.

@jmh530
Copy link
Contributor Author

jmh530 commented Mar 29, 2021

For the dmd runs, I see a line in the "Build Image" output about dub 1.22.0 being installed

#9 5.143 Using dub 1.22.0 shipped with dmd-2.093.1

Even though dmd-2.096 is getting installed and 1.23.0 is the latest release on github.

I find it a little strange that these are compiling without error, given that I assumed the problems with --single weren't even fixed until 1.24 or 1.25.

@jmh530
Copy link
Contributor Author

jmh530 commented Mar 29, 2021

Looks like the dub install version is driven by https://dlang.github.io/dub/LATEST. Not sure why that is behind the releases. Not sure how to update it.

@jmh530
Copy link
Contributor Author

jmh530 commented Mar 30, 2021

I fixed an issue related to main from before, though it still looks like an old version of dub is getting installed with ldc and it calls main instead of the UT there (I just adjusted the test so that it doesn't fail when main is called).

The only failing unittest right now seems to be an unrelated issue.

@jmh530
Copy link
Contributor Author

jmh530 commented Mar 31, 2021

Current passing version removes the dub install.sh download, as discussed here.

The ldc version is skipping unittests with dependencies.

@jmh530
Copy link
Contributor Author

jmh530 commented Apr 1, 2021

Current version installs dmd and the requested compiler. However, it seems that the dub that comes with dmd is not being used (why the ldc versions are currently failing). I don't know how to force that to be used.

@jmh530
Copy link
Contributor Author

jmh530 commented Apr 1, 2021

I tried to add dub to the path with the line below

RUN if [ \"${DLANG_EXEC}\" != \"dmd\" ] ; then \
    PATH=$(bash /tmp/install.sh get-path --dub dmd):$PATH ; \
    echo $PATH ; \
fi

but I get the output that dmd is not installed, though it looks like it was installed earlier.

#13 [10/16] RUN if [ "ldmd2" != "dmd" ] ; then     PATH=$(bash /tmp/install.sh get-path --dub dmd):/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin ;     echo /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin ; fi
#13 sha256:8cecf390b9b0cec013d74dbea120f7ff0be705952149247e1142dcb402415fe1
#13 0.314 Requested dmd-2.096.0 is not installed. Install or rerun with --install.
#13 0.315 :/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
#13 DONE 0.3s

@jmh530
Copy link
Contributor Author

jmh530 commented Apr 2, 2021

I am able to install the DMD dub. To get the path to it to use later in the Dockerfile, I needed to save it to a file (/tmp/dub_dmd_bin_path). I only want to add it to the path if the file exists (which only should happen when the dlang version isn't dmd). What I have below isn't working (and it fails early) on either dmd or ldc.

Any idea what I need to do to fix? The error message is a bit mysterious.

ENV \
  PATH=$(if [ \"${DLANG_EXEC}\" != \"dmd\" ] ; then cat /tmp/dub_dmd_bin_path ; fi):/dlang/${DLANG_VERSION}/linux/bin64:/dlang/dub:/dlang/${DLANG_VERSION}/bin:/dlang/har:${PATH} \
  LD_LIBRARY_PATH=/dlang/${DLANG_VERSION}/linux/lib64:/dlang/${DLANG_VERSION}/lib \
  LIBRARY_PATH=/dlang/${DLANG_VERSION}/linux/lib64:/dlang/${DLANG_VERSION}/lib

@jmh530
Copy link
Contributor Author

jmh530 commented Apr 4, 2021

I think I'm out of guesses at this point as to how to get this to work on ldc until the new release comes with dub 1.25.0. Basically, I was trying to download dmd separately and then add the path to dmd's dub to the path. I couldn't figure out how to get this to work in docker (the problem is that if you run a bash command, then it doesn't persist, apparently I'm not the only one with this issue). So, I tried hard coding it to the front of the PATH and that didn't work either.

I will try again when ldc-1.26 is released.

@PetarKirov
Copy link
Member

@jmh I will try to dedicate some time later this week to help you get this through the finish line.

@jmh530
Copy link
Contributor Author

jmh530 commented Apr 5, 2021

@PetarKirov I wouldn't spend too much time on it though. The issue will resolve itself soon enough.

@jmh530
Copy link
Contributor Author

jmh530 commented Apr 13, 2021

@PetarKirov Passing with ldc-beta!

@PetarKirov
Copy link
Member

@jmh530 great, than I guess we can just wait for the official 1.26 to be released and then merge this PR.

@jmh530
Copy link
Contributor Author

jmh530 commented Apr 13, 2021

@PetarKirov I'll probably squash it when the official ldc release comes out.

@jmh530
Copy link
Contributor Author

jmh530 commented Apr 28, 2021

New LDC released today, waiting on maintainer to approve running workflow to ensure it passes.

@jmh530 jmh530 changed the title WIP: Add dub test Add dub test May 1, 2021
@jmh530
Copy link
Contributor Author

jmh530 commented May 1, 2021

@PetarKirov @Geod24 Can I get workflow approval to test the new LDC release?

@Geod24
Copy link
Member

Geod24 commented May 1, 2021

Done

@jmh530
Copy link
Contributor Author

jmh530 commented May 1, 2021

@Geod24 Thanks! It's passing now!

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.

Thanks for your work on this! Just one comment, rest LGTM!

envwrapper Outdated
#!/bin/bash
TEMP=$(cat /tmp/dub_dmd_bin_path)
export DUB_DMD_BIN_PATH=$(TEMP:+:)
$*
Copy link
Member

Choose a reason for hiding this comment

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

You might want to exec here (and add newline) so that signals are passed down properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That file isn't used anymore. I had forgotten to remove. Needs another approval to run CI again.

@Geod24 Geod24 merged commit ac80fa8 into dlang-tour:master May 1, 2021
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.

3 participants