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

Proposal: Use DUB, so that command to run the tests is dub test #30

Merged
merged 8 commits into from
Jun 4, 2017

Conversation

petertseng
Copy link
Member

@petertseng petertseng commented Feb 12, 2017

Problem statement

Currently, the command I have to build an exercise and run its tests is:

dmd *.d -de -w -unittest && ./$(basename $(pwd) | tr - _)

As it stands, this is not the worst experience in the world - it is a single command that I can run that works for all exercises. But I have to look it up again when I want to run it (or keep a special alias in my shell rcfile).

I really wish this were easier.

Proposal

If this track were to use DUB projects, then the command to run the tests for an exercise becomes:

dub test

I know this is a fairly significant proposal (many files changes in this PR), but showing what the track would look like if the proposal were to be accepted is the most reasonable way to allow the changes to be evaluated. Each commit represents a single step applied to all exercises.

An enhancement to this track's Travis CI is included has been merged in #31. The CI has been modified to add assurance that, as advertised, dub test is sufficient to run the tests under the proposal.

Decisions made

https://code.dlang.org/getting_started tells us that either source or src are acceptable. I arbitrarily chose source (because dub init uses it). If there is a preference for src, please say so.

dub.sdl vs dub.json? I don't care much, and we only configure the name and build requirements, nothing very complex. So again, I arbitrarily chose sdl: fewer bytes.

Both these decisions were arbitrary and not because the chosen option is clearly superior. If there is a future reason to choose the other option instead, feel free to choose it!

buildRequirements "disallowDeprecations" in all dub.sdl files, because the behaviour recommended to students before this PR (in docs/TESTS.md) was to compile with -de and -w flags, and disallowDeprecations noted in https://code.dlang.org/package-format?lang=sdl#build-requirements keeps this behaviour (use a dub test -v to verify this - the dmd command invoked will include both flags, whereas it only includes -w and NOT -de if the line is absent).

Disadvantages

The obvious disadvantage to this approach is that we are now asking students to install DUB.
Slight mitigation of disadvantage: Of course, it would still be possible for a student to get along without DUB and just run dmd as before, if the student desires it.

This is the first time I have ever used DUB. So, it is possible I miss something.

Remaining work

I am aware of at least the following work to be done before this PR can be merged. I'm aware, but have not done it yet, because I need an idea of whether the proposal will be accepted before it is worth it to work on it.

  • docs/INSTALLATION.md must explain how to install DUB. I plan to use https://code.dlang.org/download to help.
  • docs/TESTS.md must ask student to use dub test instead of dmd ....
  • README.md must ask for the new directory structure, with source

Other tracks

I see that other tracks have moved to this sort of model in the past as well, with the same advantage of a single command that the student can use to run the test:

Tracks that explicitly choose to not use a particular build system:

Full list at exercism/discussions#117

@petertseng
Copy link
Member Author

If @exercism/dlang will not review this, then what will we do?

@petertseng petertseng mentioned this pull request May 7, 2017
@kytrinyx
Copy link
Member

kytrinyx commented Jun 2, 2017

@petertseng I like the proposal of adding DUB. It sounds like a much better user experience.

I say go for it. I'll merge it when you're ready.

@petertseng
Copy link
Member Author

ETA: Some time during this weekend.

@petertseng
Copy link
Member Author

petertseng commented Jun 2, 2017

an intermediate step has been completed (merge conflict with #31 resolved)

git diff 877dd50 2728bbe is as I expect

step 3a and step 3b collapsed into step 3 since step 3b was handled by #31 instead.

everything looks correct with the rebase

But we are still not yet ready to merge.

  • The "Remaining work" items must be done as well.
  • It is likely that react: add to track #32 and leap: add to track #35 will be merged first, adding two new exercises. They must also receive the same treatment as other exercises, as this should be a track-wide move. ✔️

@petertseng petertseng changed the title Proposal: Use DUB, so that command to run the tests is dub test [Do not merge] Proposal: Use DUB, so that command to run the tests is dub test Jun 2, 2017
Dub expects the source files to be in `source/` so that it will find
them when running `dub test`.

Of the possible choices `source` and `src`, `source` was chosen
arbitrarily. The alternative `src` is equally acceptable.
@petertseng petertseng changed the title [Do not merge] Proposal: Use DUB, so that command to run the tests is dub test Proposal: Use DUB, so that command to run the tests is dub test Jun 4, 2017
@petertseng
Copy link
Member Author

petertseng commented Jun 4, 2017

I would say this is mergeable as-is now (unless y'all think there can be improvements in the docs), but if it is merged, I will feel obligated to open an issue about whether we can preserve the "warnings -> errors" and "disallow deprecations" behaviours, since that is a remaining open question.

It's not my highest priority, since the default behaviours are "display warnings but do not stop compilation" and "warn on deprecations", which is at least acceptable even if it is not as strict.

Note: the file that gets inserted in every readme (SETUP.md) does not specifically name a command to run the tests, instead deferring to the website, so it did not need to be changed as part of the docs commit (but docs/TESTS.md did)

@petertseng
Copy link
Member Author

Experiments (I used the JSON format since it's certain I got the syntax right, want as few uncertain variables as possible):

{
  "name": "bob",
  "buildOptions": ["warningsAsErrors", "deprecationErrors"]
}

Result:

## Warning for package bob ##

The following compiler flags have been specified in the package description
file. They are handled by DUB and direct use in packages is discouraged.
Alternatively, you can set the DFLAGS environment variable to pass custom flags
to the compiler, or use one of the suggestions below:

warningsAsErrors: Use "buildRequirements" to control the warning level
deprecationErrors: Use "buildRequirements" to control the deprecation warning level

Generating test runner configuration 'bob-test-library' for 'library' (library).
Performing "unittest" build using dmd for x86_64.
bob ~master: building configuration "bob-test-library"...
Linking...
Running ./bob-test-library
All unit tests have been run successfully.

Huh okay. What about using buildRequirements then? I don't see a way to disallow warnings altogether, but I can disallow deprecations:

{
  "name": "bob",
  "buildRequirements": ["disallowDeprecations"]
}
Generating test runner configuration 'bob-test-library' for 'library' (library).
Performing "unittest" build using dmd for x86_64.
bob ~master: building configuration "bob-test-library"...

Linking...
Running ./bob-test-library
All unit tests have been run successfully.

(Actually, running with dub test -v shows that the -w and -de flags are being passed here, so it looks like this buildRequirements is what we would want if we wanted to preserve the old behaviours)

@petertseng
Copy link
Member Author

petertseng commented Jun 4, 2017

(and running with NO buildRequirements does pass the -w flag (but not -de), so maybe https://code.dlang.org/package-format?lang=sdl#build-options is inaccurate about what is enabled by default, or the defaults are different for unittest mode)

@petertseng
Copy link
Member Author

petertseng commented Jun 4, 2017

OK, using DUB's buildRequirements "disallowDeprecations" in all dub.sdl files to preserve the desired behaviours.

This means all open questions are resolved, and I can't think of more changes I would like to make.

A default dub project adds many more attributes, but through
experimentation I see that only the name is necessary.

Then, we add `buildRequirements "disallowDeprecations"` because the
behaviour recommended to students before this PR (in `docs/TESTS.md`)
was to compile with `-de` and `-w` flags, and `disallowDeprecations`
keeps this behaviour.

Use a `dub test -v` to verify this - the `dmd` command invoked will
include both flags, whereas it only includes `-w` and NOT `-de` if the
line is absent.

Reference for `buildRequirements`
https://code.dlang.org/package-format?lang=sdl#build-requirements

Note that using `buildOptions` to explicitly set `-de` will instead give
you a warning that you should use `buildRequirements` instead!
When dub compiles these files, it will treat them as libraries, which
means it will insert a main for us. If we define a main as well, we will
get "Error: only one main allowed"
Markdown renderers will usually want at least two spaces to render as a
sub-bullet.
The hyphens get replaced with underscores, not the other way around!
@kytrinyx
Copy link
Member

kytrinyx commented Jun 4, 2017

I don't think we need an issue for this. If it becomes annoying later we can have the discussion.

Thank you so much for taking the time to do this so thoroughly!

@kytrinyx kytrinyx merged commit 65d5629 into exercism:master Jun 4, 2017
@petertseng petertseng deleted the dub-proposal branch June 4, 2017 03:55
@petertseng
Copy link
Member Author

Thanks for looking! I had not expected you would since you said at http://tinyletter.com/exercism/letters/exercism-behind-the-scenes-sustainability you won't be paying attention. But that was six weeks ago, so perhaps things have changed since then and/or something freed up more time.

@kytrinyx
Copy link
Member

kytrinyx commented Jun 4, 2017

@petertseng I had a vacation :-)

Also, I'm spending a fair amount of time on figuring out what the current status of everything is, and clearing out things that have been waiting around, as part of deciding how to proceed. So the answer is: don't expect me to be looking, but I will probably end up doing some deep dives in various areas as I work my way through it all.

@petertseng petertseng mentioned this pull request Sep 21, 2017
#### Mac OS X

* The [DUB website's download page](https://code.dlang.org/download) has OS X binaries.
* or using [Homebrew](http://brew.sh/): brew install dmd
Copy link
Member Author

Choose a reason for hiding this comment

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

nope, this is in the installing DUB section, so the command should say dub. Let #59 fix it.

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.

2 participants