implement "minDubVersion" field #1532

rtbo · 2018-08-18T19:02:34Z

dlang-bot · 2018-08-18T19:02:35Z

Thanks for your pull request and interest in making D better, @rtbo! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

WebFreak001

Looking good, add a changelog entry and imo this can be merged

WebFreak001 · 2018-08-19T16:28:31Z

let's speed this up a little, I think this is a fairly straightforward change that wouldn't get a lot of negative feedback and formally everything is there: changelog, tests, code

wilzbach

I like the idea, but please add a real test script for this.

wilzbach · 2018-08-20T12:17:09Z

source/dub/package_.d

+			static assert(isValidVersion(dv));
+
+			enforce(compareVersions(dv, mdv) >= 0,
+				"dub-"~dv~" does not comply with minDubVersion specification: "~mdv);


Maybe state something like: "please upgrade your D installation to at least dub X.Y.Z"?

did this:

dub-1.10.0 does not comply with minDubVersion specification: 99.0.0. Please consider upgrading your dub installation.

wilzbach · 2018-08-20T12:19:27Z

test/issue1531-min-dub-version/source/app.d

@@ -0,0 +1,3 @@
+module app;
+
+void main() {}


This test is never run!
It needs a shell test (see the .sh scripts in the test folder).

run_unittest.sh also iterates the directories under test.
i.e. the package is built and build passes or fail depending on the minDubVersion specified. (checked out a one that passes)

$ DUB=... test/run_unittest.sh Testing summary: [INFO] Building /home/remi/dev/dlang/dub/test/issue1531-min-dub-version/...

Now I guess I can have a script to test for both success and failure.

wilzbach · 2018-08-20T12:20:00Z

changelog/minDubVersion.dd

@@ -0,0 +1,6 @@
+Dub now supports a $(D_INLINECODE "minDubVersion") field, that will abort build if


Just using backticks would be fine too...

wilzbach · 2018-08-20T12:20:41Z

changelog/minDubVersion.dd

+the user does not have a recent enough version of dub and miss essential feature
+to perform the build.
+-------
+    "minDubVersion": "1.10"


minDubVersion can be added to a projects's dub.json:

{ ... "minDubVersion": "1.10" ... }

Or the respective dub.sdl:

minDubVersion "1.10"

wilzbach · 2018-08-20T12:23:00Z

formally everything is there: changelog, tests, code

No this doesn't get tested at the moment.

AntonMeep · 2018-08-30T17:16:30Z

Discussing this in #1021 it got me thinking about minDubVersion. Maybe this is not the best name, since in the (bright) future we would have DMD following semver which will make it possible to add the same version-checking feature for dub and we would want to add minDmdVersion, minLdcVersion, minGdcVersion.

Why don't we use some new namespace for that, for example:

{
  "name": "test",
  ...
  "devDependencies": {
    "dmd": "~>2.81.0",
    "dub": "~>1.10.0"
  }
}

It could be requirements or something like that. Actually, since LDC already follows semver and GDC too (well, when it gets merged into GCC), it could be implemented too.

@wilzbach, @WebFreak001 what's your opinion on that? Otherwise, this PR is fully complete with test and changelog.

WebFreak001 · 2018-09-06T08:04:08Z

I like the idea but something else than devDependencies would be good. Node already uses it, making it even harder to differentiate between the files if they are called package.json and because they are not actual dependencies but only for checking. requirements would be good as it would also not imply that all dmd, ldc and gdc are required to be installed for the project to build if the project specifies versions for all 3 of them

wilzbach

LGTM minus:

update to the docs (dub-docs)
mutation of checked in files (in theory the test suite might stop any second and leave the source directory in a dirty state)

Feel free to merge this once these nits have been addressed.

wilzbach · 2018-09-20T17:02:30Z

test/issue1531-min-dub-version.sh

+rm -rf $DIR1531/.dub/
+rm -rf $DIR1531/test-application*
+
+sed -i 's/^minDubVersion ".*"$/minDubVersion "1.0"/' ${DIR1531}/dub.sdl


It's not a good idea to mutate files that are checked in the source tree by the testsuite, better put the dub.sdl into a temporary directory.
Also since the last version dub supports passing files in from stdin, so you could use --single plus pass the file in via stdin which would eliminate the need for a temporary directory entirely.

wilzbach · 2018-09-20T17:03:37Z

test/issue1531-min-dub-version.sh

+
+sed -i 's/^minDubVersion ".*"$/minDubVersion "99.0"/' $DIR1531/dub.sdl
+! ${DUB} run --root ${DIR1531} || die "Did pass minDubVersion \"99.0\"!"
+


Maybe a good idea to add one test that works, e.g. with 2.068 or similar?

What do you mean by this? I already test for success and for failure.

see dlang/dub#1532

WebFreak001 · 2018-10-02T15:53:58Z

Seems to be good now, will merge if there are no more objections

PetarKirov · 2018-10-03T17:17:16Z

I think this change was aiming in the right direction, but it has one significant drawback - it doesn't integrate through the established dependency system.
dmd and dub are build dependencies just like any other package yours depends on, because without all the right versions of all of the above you can't build your package.
Not being able to specify "dub": "~>1.0" means that we won't be able to make a breaking change to dub and release version dub 2.0, since any package with "minDubVersion": "1.10" will happily accept 2.0.

PetarKirov · 2018-10-03T17:20:50Z

I mean essentially what @ohdatboi was proposing, but either putting dub, dmd, ldc, etc. next to the other dependencies, or having a separate toolchainDependencies section.

MartinNowak · 2018-10-05T17:36:50Z

Compatibility is a reasonable argument for ~> style dependencies.
How about coming up with a PR that implements this instead of minDubVersion?

rtbo · 2018-10-06T07:45:00Z

I agree with this. We should have this done before the first v1.12 beta is out (in a week or so?), or otherwise have minDubVersion reverted.

MartinNowak · 2018-10-25T19:00:19Z

Kudos for taking action on this, was a bit sad to see this leaking into the release. Very glad to see #1571 :).

wilzbach · 2018-10-25T21:58:47Z

@MartinNowak this shouldn't be part of the real release anymore as it's reverted: #1581

rtbo · 2018-10-25T22:35:22Z

This has leaked in changelog and beta1unfortunately.

wilzbach · 2018-10-25T22:41:54Z

Yes, and that's not ideal, but it won't be part of the official release and that's what really matters.

rtbo added 2 commits August 18, 2018 21:00

implement minDubVersion check

db5b906

fix dlang#1531

test minDubVersion

83e7bf2

fix test 1531

bdfb52f

WebFreak001 requested changes Aug 19, 2018

View reviewed changes

rtbo added 2 commits August 19, 2018 18:20

changelog

19b9a62

save function call

140501b

WebFreak001 approved these changes Aug 19, 2018

View reviewed changes

WebFreak001 added the 72h no objection -> merge label Aug 19, 2018

wilzbach requested changes Aug 20, 2018

View reviewed changes

wilzbach removed the 72h no objection -> merge label Aug 20, 2018

rtbo added 3 commits August 20, 2018 22:13

better changelog

0c86574

better error message

954946b

better test

19e0790

rtbo force-pushed the min_dub_version branch from 1d04d3f to 19e0790 Compare August 20, 2018 22:20

wilzbach approved these changes Sep 20, 2018

View reviewed changes

implement test 1531 from stdin

59dac6e

rtbo added a commit to rtbo/dub-docs that referenced this pull request Sep 30, 2018

document minDubVersion field

5377b6a

see dlang/dub#1532

rtbo mentioned this pull request Sep 30, 2018

document minDubVersion field dlang/dub-docs#5

Merged

WebFreak001 added the 72h no objection -> merge label Oct 2, 2018

wilzbach merged commit bb62065 into dlang:master Oct 2, 2018

rtbo mentioned this pull request Oct 7, 2018

toolchainRequirements recipe support #1571

Merged

wilzbach mentioned this pull request Oct 19, 2018

Revert "implement "minDubVersion" field" #1580

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement "minDubVersion" field #1532

implement "minDubVersion" field #1532

rtbo commented Aug 18, 2018

dlang-bot commented Aug 18, 2018

WebFreak001 left a comment

WebFreak001 commented Aug 19, 2018

wilzbach left a comment

wilzbach Aug 20, 2018

rtbo Aug 20, 2018

wilzbach Aug 20, 2018

rtbo Aug 20, 2018

wilzbach Aug 20, 2018

wilzbach Aug 20, 2018

wilzbach commented Aug 20, 2018

AntonMeep commented Aug 30, 2018

WebFreak001 commented Sep 6, 2018

wilzbach left a comment

wilzbach Sep 20, 2018

wilzbach Sep 20, 2018

rtbo Sep 30, 2018

WebFreak001 commented Oct 2, 2018

PetarKirov commented Oct 3, 2018

PetarKirov commented Oct 3, 2018

MartinNowak commented Oct 5, 2018

rtbo commented Oct 6, 2018

MartinNowak commented Oct 25, 2018

wilzbach commented Oct 25, 2018

rtbo commented Oct 25, 2018

wilzbach commented Oct 25, 2018

		@@ -0,0 +1,6 @@
		Dub now supports a $(D_INLINECODE "minDubVersion") field, that will abort build if


		sed -i 's/^minDubVersion ".*"$/minDubVersion "99.0"/' $DIR1531/dub.sdl
		! ${DUB} run --root ${DIR1531} \|\| die "Did pass minDubVersion \"99.0\"!"

implement "minDubVersion" field #1532

implement "minDubVersion" field #1532

Conversation

rtbo commented Aug 18, 2018

dlang-bot commented Aug 18, 2018

WebFreak001 left a comment

Choose a reason for hiding this comment

WebFreak001 commented Aug 19, 2018

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Aug 20, 2018

AntonMeep commented Aug 30, 2018

WebFreak001 commented Sep 6, 2018

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WebFreak001 commented Oct 2, 2018

PetarKirov commented Oct 3, 2018

PetarKirov commented Oct 3, 2018

MartinNowak commented Oct 5, 2018

rtbo commented Oct 6, 2018

MartinNowak commented Oct 25, 2018

wilzbach commented Oct 25, 2018

rtbo commented Oct 25, 2018

wilzbach commented Oct 25, 2018