Feature: git submodule support via path selection #1802

FeepingCreature · 2019-11-22T14:23:40Z

Second try! (ping @wilzbach @atilaneves )

This approach relies on hardcoding the submodules in dub.selections.json and thus requires some more user intervention. The idea here is to extend dependencies to include both path and version, and for dub.selections.json determine the version from the SCM. Then, instead of overwriting the dependencies with dub.selections.json we check if the selected package actually matches the version requirements, which is probably something we should do anyways - if dub.selections.json says 2.3.4 but the package requires >=2.4.0 it's highly questionable to continue with 2.3.4 anyways.

Open question: how does this interact with dub upgrade? How should it?

Open task: write test for it.

See #1780 for previous debate.

dlang-bot · 2019-11-22T14:23:41Z

Thanks for your pull request and interest in making D better, @FeepingCreature! 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 coverage diff by visiting the details link of the codecov check)
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.

CyberShadow · 2019-11-22T15:57:58Z

source/dub/package_.d

-	if (!existsFile(git_dir) || !isDir(git_dir.toNativeString)) return null;
+	if (!existsFile(git_dir)) return null;
+	if (!isDir(git_dir.toNativeString)) {
+		auto gitredirect = readText(git_dir.toNativeString).strip();


You want .chomp not .strip, but really, you should not be parsing git's files by hand. The correct way is to invoke git to query information.

If invoking git is not viable (e.g. too slow on Windows), then code that messes with git's internals ought to be encapsulated in its own module.

Commit message should say that this is for absorbed submodules (i.e. those for which .git is a file).

Could you please paste your IRC comment for how to do this here? My backlog doesn't go that far.

I think this is the relevant line: [2019-11-22 16:07:24.606] <CyberShadow> git -C path/to/submodule rev-parse --git-dir

Fixed? It now uses git rev-parse on non-Windows. I've left the previous code behind version(Windows) inside the module because I don't think that that refactoring is in-scope for this PR, though I agree it should be done. The Windows implementation should probably be split out completely so it doesn't confuse up the code. Raise an issue please?

Fixed via separate PR.

CyberShadow · 2019-11-22T16:04:54Z

source/dub/project.d

+					vspec = vspec.merge(selection);
+					enforce(vspec != Dependency.invalid, format(
+						"Selected package %s doesn't match %s(%s) of %s",
+							selection, dep.name, dep.spec, pack.name));


If I understand this correctly, this will probably break the way I use Dub + submodules where I bind-mount the submodule to its "real" location (i.e. bindfs -n ~/work/ae lib/ae) - since the main directory is on master, it won't match the dub.selections.json version. That's probably fine though. If there isn't a way to override it (local dub.selections.json override?) then maybe it would be nice if there was a way.

Commit message should conform to git conventions (first line under 60-80 characters).

Should work. Branches are excluded from the override, for pretty much that reason that it would make incremental development impossible, which is exactly the thing submodules/bind mounts are for.

wilzbach · 2019-11-25T09:37:35Z

Thanks!

Open question: how does this interact with dub upgrade? How should it?

I guess a user would expect dub upgrade to at least preserve the current selection of path-based dependencies and upgrade dependencies from the registry. WDYT?

FeepingCreature · 2019-11-25T09:41:04Z

I guess a user would expect dub upgrade to at least preserve the current selection of path-based dependencies and upgrade dependencies from the registry.

The question is largely whether it should be smart enough to upgrade git submodules. Since submodules are a form of externally tracked selection, IMO dub upgrade should upgrade them for the same reason and by the same logic that it upgrades dub.selections.json - they're in the selections, to dub that should mean it has ownership over their version state.

atilaneves · 2019-11-25T11:11:36Z

I don't think I saw a reply to a similar question I asked in the last PR: why is this needed when using git submodules works today by specifying a path instead of a version number?

FeepingCreature · 2019-11-25T11:28:57Z

Submodules "work" today with dub, in that you can include submodules and build running code. Submodules don't work in the sense that if you include submodules via path=, dub bypasses all version checking logic, which is the one major thing that dub can do that rdmd doesn't do more simply and with much less effort. The point of this change is to enable submodules containing dub packages to participate in version checking.

CyberShadow · 2019-11-25T15:11:23Z

Open question: how does this interact with dub upgrade? How should it?

Would it be appropriate for dub upgrade to change dub.selections.json to match the submodule state? Then, upgrading a submodule dependency would involve 1) checking out a new tag in the submodule, then 2) running dub upgrade in the parent.

FeepingCreature · 2019-11-25T15:27:46Z

Would it be appropriate for dub upgrade to change dub.selections.json to match the submodule state? Then, upgrading a submodule dependency would involve 1) checking out a new tag in the submodule, then 2) running dub upgrade in the parent.

Put that on the list of "why is path=" insufficient - path= precludes version=. So there's nothing in dub.selections.json to adjust. The state is fully stored in the checked-out branch. So even putting aside how this kind of degenerates into weird workflow where dub is basically one step away from telling you the commands to type in, instead of doing what it knows needs doing, it wouldn't work because there's nothing in dub.selections.json to match the submodule state.

CyberShadow · 2019-11-25T15:37:46Z

I see. I think that's just fine if the dependency is in a submodule, as Git already holds the exact version of the dependency, so copying it in dub.selections.json is technically redundant. It might still be useful if the built project is not in git (e.g. the user downloaded and unpacked the tarball / ZIP file, then did the same by hand with any dependencies / submodules).

As such, I think simply doing nothing / printing a message like Skipping path-specified dependency libfoo at lib/foo would be appropriate. Considering the above, maybe it would also be useful to print a message when processing path-specified dependencies which are not git submodules (so we have no way at all to check their version), though that would probably get annoying for those who depend on this kind of workflow for normal development.

FeepingCreature · 2019-11-25T15:57:15Z

But I mean, just to put it from my perspective again - we're not using submodules because we want dub to leave them alone, we're using submodules because it's the only viable way of simultaneously co-developing two repositories with dub in a CI-capable fashion unless I manage to convince whoever did it to un-deprecate branches. So this kind of "this is a submodule, I'm going to tactfully ignore it because I assume you know what version you've checked out" logic is actively at odds with what we are actually interested in dub for. For that matter, I'm not sure that anybody has a workflow that relies on using submodules to force dub to ignore versions, one of its core competencies. I hope the notion to have dub upgrade automatically update submodule state makes more sense from that perspective? We don't want to use submodules as an addon to dub, we want to use submodules with dub. All the way.

rikkimax · 2019-11-25T16:05:50Z

Shouldn't dub upgrade call out into a user-defined program to provide new entries?
This way the VCS is entirely configurable along with what submodules to add (it can also do checkout's ext. too).

FeepingCreature · 2019-11-25T16:09:42Z

I think a plugin system, which is what this would amount to, is kind of a big task. So like, "should" maybe? Though I can't imagine how it'd work. But that's something I personally have no use for with dub, and would probably push the task into "too big to bother" for me.

rikkimax · 2019-11-25T16:35:35Z

Nah, just a regular command like dub already supports e.g. preGenerateCommands, let them return a JSON string which has the paths ext. all specified via stdout.

No need to make it complicated :)

FeepingCreature · 2019-11-25T17:40:46Z

Would need a command to switch to a new state too. Does get a bit complicated. Are there even other VCS that have submodule equivalents?

rikkimax · 2019-11-26T04:20:17Z

Mercurial has something similar: https://www.mercurial-scm.org/wiki/Subrepository

I don't like the idea of baking a specific VCS behavior into dub itself. Hence the command support.

Sadly we have to bake into the registry specific providers because of no common API provided. Without it understanding specific VCS systems.

FeepingCreature · 2019-11-26T05:52:17Z

Well, that's a matter for that PR if/when I end up making it. Let's table that idea for this PR.

atilaneves · 2019-11-27T10:03:59Z

Submodules "work" today with dub, in that you can include submodules and build running code. Submodules don't work in the sense that if you include submodules via path=, dub bypasses all version checking logic, which is the one major thing that dub can do that rdmd doesn't do more simply and with much less effort. The point of this change is to enable submodules containing dub packages to participate in version checking.

Why would one need version checking logic for a git submodule? Their versions are pinned by commit and have to be changed manually.

FeepingCreature · 2019-11-27T12:23:41Z

Why would one need version checking logic for a git submodule? Their versions are pinned by commit and have to be changed manually.

We need some way to decide the question "which release version can we pull the submodule up to". Right now, this question is answered by the mechanism of "update the submodule to the newest version and see if you get compilation errors." This is a terrible mechanism for many reasons. We would like a better one.

Dub is the mechanism by which D projects are supposed to verify dependencies. The logical, 2+2=4 tier, response is to ask "can we make dub work for submodules too?" Hence this PR.

The alternative is for us to straight up ditch submodules and go back to development branches. Branches are deprecated in dub. I don't see how we're supposed to move forward with dub in any situation where we have to co-develop two repositories in a way that has to build on CI. (Branches would be a step down from submodules, as we'd no longer have reproducibility. But still a step up from the current 'just pull it up and see if it works, lol.')

atilaneves · 2019-11-27T15:09:57Z

We need some way to decide the question "which release version can we pull the submodule up to".

I don't understand why we need that. Either one uses a version specification as usual, or one uses a git submodule. If said submodule had version tags, one could already just use it normally.

This is a terrible mechanism for many reasons. We would like a better one.

How is it worse than "dub upgrade and see if the build still passes"?

The alternative is for us to straight up ditch submodules and go back to development branches

I don't see how that follows.

I don't see how we're supposed to move forward with dub in any situation where we have to co-develop two repositories in a way that has to build on CI.

The way I do it now - change the dependency on a version to a path dependency, commit, change the dependency until things work, git rebase -i to modify the path dependency commit to a version update.

atilaneves · 2019-11-27T15:10:04Z

Having said all this, I remember now that when I tried adding a git submodule to a dub package, it worked, but not if any other dub package depended on it. That I'd be in favour of fixing.

FeepingCreature · 2019-11-27T15:29:42Z

If said submodule had version tags, one could already just use it normally.

As a dub dependency? Yes, but again - not with branches, unless one was to specifically use a deprecated dub feature. Which realistically isn't going to ever go away, but it's still bad workflow.

So, again, the problem arises when you have two repos that have to coevolve, for instance because you're trying to introduce some library functionality that is only meaningful in the context of the other repository. (But you still want dependency checking the rest of the time.)

I don't understand why we need that.

Sorry, to restate - I didn't mean "we as in the D community", I meant specifically "us at Funkwerk, who use submodules pretty widely."

How is it worse than "dub upgrade and see if the build still passes"?

Entire point of dub version checking is that if you did semver correctly, you don't need to build to see if it works because dub will select versions that are known to work via dependency information! That's one of dub's major value adds over rdmd...

I think part of the confusion is that there's two separate major usecases here.

The first is "We want the newest versions of those seven repos that are intercompatible." That's dub upgrade's job.

The second is "We want this repo to be available from a branch, and we want to incrementally change it and use those changes in another repo, and validate that other repo on CI." That's submodule's job. In the limited cases where we use dub with internal repos right now, I would have to use add-local for this (aside the efficient but ugly hack of editing $HOME/.dub directly), but that's unwanted global state liable to cause problems for builds in different repositories that suddenly see a different package.

The problem is unifying those two usecases, without having to continually switch between submodules and dub.json dependencies. (Which would lead to a lot of git breakage, and also be extremely annoying.)

Basically, I want to use submodules as a committable, repository-specific, equivalent of add-local, that can be used in the same way as add-local but works on CI unlike add-local and behaves otherwise like regular dub if a tag is checked out.

If I pull up some dependency, my versions won't work. The goal is: I want D tooling to tell me this in the form of "Package X required Dependency Y>9.1.0, but Y 8.7.0 is selected."

I don't want D tooling to tell me this in the form of "{three pages of linker errors}", or "{cryptic template invocation error}". Or, God forbid, a crash on a customer system due to incompatible runtime behavior.

My impression was that dub was a tool that attempted to give me this. So I don't understand all the questions of "why do you need version checking with this?" What would I even want dub for if not for version checking?

atilaneves · 2019-11-27T17:50:42Z

I have the feeling that we're not understanding each other. I'm trying my best to understand what you want to enable that doesn't work now.

I noticed now rereading that I may have understood completely what you're trying to do - I thought we were talking about git submodules, not dub ones.

I suggest adding a test case enabled by the diff - even after rereading everything I don't quite get what it's supposed to do.

FeepingCreature · 2019-11-28T07:18:13Z

I noticed now rereading that I may have understood completely what you're trying to do - I thought we were talking about git submodules, not dub ones.

We are.

Okay, let's give a practical example. All names and project versions are fake, but this is structurally equivalent to problems that have come up in practice with the serial numbers filed off.

Note that all of these projects are simultaneously their own freestanding projects and act as submodules in five to ten other projects.

So say we have "utilities" (the obligatory "internal utilities lib" every organization accumulates) at 5.2.3 and our event-store client is at 1.0.3. So "utilities" is a submodule in "event-store", because we needed to develop and test this fix in the context of event-store's source code, so we've just tagged utilities as 5.2.3 and set a dub dependency in "event-store" on ^5.2.3.

Now, say we have a project that tracks the position of a train and enforces invariants such as that positions shouldn't change more than a hundred km between position updates, or whatever. And this is the project that originally triggered the work on event-store, which is a submodule in train-position, which has a submodule for utilities but the actual submodule used to link train-position is the submodule of utilities placed directly in train-position (because obviously train-position needs its own copy). And this is now implicitly utilities: ^5.2.3 via the dependency on the just tagged event-store: ^1.0.3 in, say, train-position 2.1.3.

However, train-position also exposes a library subproject, because other services need to know how to talk to it and how to read events from its domain. So train-position:library is also referenced in, I don't know, train-arrival or whatever, which determines when a train has arrived in a station. And train-arrival has just switched from train-position 0.7.4 to 2.1.3 but is still on utilities 3.4.4. (And these are all recursive submodules, for the reason outlined in my previous comment, but usually you'll only have the first level checked out, for the reason of O(n^2) and "Gigabit LAN isn't that fast if you have to clone like fifty repos.")

How does the user get informed, when building train-arrival ~master where he just updated the train-position submodule to 2.1.3, that utilities 3.4.4 is too old?

With this patch, what happens is simply exactly that. Dub determines the version of train-position to be 2.1.3 by inspecting its submodule state, checks the dub.json, sees that it depends on event-store ^1.0.3, sees that event-store is available as a submodule as "1.0.3", checks its dub.json, sees that utilities in there is ^5.2.3; determines that utilities is also a submodule with (by git inspection) version 3.4.4, and highlights a version error between "train-arrival" (implicitly depends on utilities: ==3.4.4 via the merge with dub.selections.json, see patch), and "event-store" (explicitly depends on utilities: ^5.2.3).

Selected package 3.4.4 @/home/example/code/train-arrival/.git_repos/utilities doesn't match utilities(^5.2.3) of event-store:library

Without this patch, dub just shrugs its shoulders and goes "eh, I guess they're all ~master. And anyways since you checked out utilities 3.4.4, I guess that means you want utilities 3.4.4, even though this makes the dependency graph inconsistent. I guess that's your problem now. Here's two pages of compile errors! HTH HAND"

…ons.json specified versions actually match the dependency requirements. For path selections, get the required version from the SCM if it's a tag.

…lly interpreting the `.git` file, except on Windows.

FeepingCreature · 2019-12-03T05:45:33Z

@atilaneves Does that make it clearer?

atilaneves · 2019-12-04T10:39:45Z

I think so. Thanks!

Geod24 · 2020-12-07T17:41:19Z

Do we still need this, now that support was added ?

FeepingCreature · 2020-12-09T08:26:57Z

Yeah, gonna close it. Whether this needs doing or not, nothing else is likely to come of this PR.

Thanks for reminding. :)

FeepingCreature force-pushed the feature/dub-submodule-support-via-path-selection branch from 515bf24 to d3b7954 Compare November 22, 2019 14:28

FeepingCreature mentioned this pull request Nov 22, 2019

Revert "Add flag --submodules to add git submodules in the project folder as available packages." #1801

Merged

CyberShadow reviewed Nov 22, 2019

View reviewed changes

FeepingCreature changed the base branch from revert-1780-feature/git-submodules-as-local-package to master November 25, 2019 09:49

FeepingCreature changed the title ~~Feature/dub submodule support via path selection~~ Feature: git submodule support via path selection Nov 28, 2019

FeepingCreature added 3 commits November 28, 2019 13:17

fix dependency merging of "0.x.0" and "^0.x.0"

ae8442b

allow merging of dependency paths

1c98c21

allow identifying the version of git submodules

b990d9e

FeepingCreature added 2 commits November 28, 2019 13:20

Instead of overwriting project dependencies, enforce that dub.selecti…

8fd76ce

…ons.json specified versions actually match the dependency requirements. For path selections, get the required version from the SCM if it's a tag.

Determine git submodule version with git rev-parse instead of manua…

844c9fa

…lly interpreting the `.git` file, except on Windows.

FeepingCreature force-pushed the feature/dub-submodule-support-via-path-selection branch from 473d69c to 844c9fa Compare November 28, 2019 12:26

Geod24 mentioned this pull request Dec 27, 2019

Support git submodules as dependencies #1840

Open

FeepingCreature closed this Dec 9, 2020

Feature: git submodule support via path selection #1802

Feature: git submodule support via path selection #1802

Conversation

FeepingCreature commented Nov 22, 2019 • edited Loading

dlang-bot commented Nov 22, 2019

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

FeepingCreature Nov 28, 2019 • edited Loading

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 Nov 25, 2019 • edited Loading

FeepingCreature commented Nov 25, 2019

atilaneves commented Nov 25, 2019

FeepingCreature commented Nov 25, 2019 • edited Loading

CyberShadow commented Nov 25, 2019

FeepingCreature commented Nov 25, 2019 • edited Loading

CyberShadow commented Nov 25, 2019

FeepingCreature commented Nov 25, 2019 • edited Loading

rikkimax commented Nov 25, 2019

FeepingCreature commented Nov 25, 2019

rikkimax commented Nov 25, 2019

FeepingCreature commented Nov 25, 2019 • edited Loading

rikkimax commented Nov 26, 2019

FeepingCreature commented Nov 26, 2019 • edited Loading

atilaneves commented Nov 27, 2019

FeepingCreature commented Nov 27, 2019 • edited Loading

atilaneves commented Nov 27, 2019

atilaneves commented Nov 27, 2019

FeepingCreature commented Nov 27, 2019 • edited Loading

atilaneves commented Nov 27, 2019

FeepingCreature commented Nov 28, 2019 • edited Loading

FeepingCreature commented Dec 3, 2019 • edited Loading

atilaneves commented Dec 4, 2019

Geod24 commented Dec 7, 2020

FeepingCreature commented Dec 9, 2020

FeepingCreature commented Nov 22, 2019 •

edited

Loading

FeepingCreature Nov 28, 2019 •

edited

Loading

wilzbach commented Nov 25, 2019 •

edited

Loading

FeepingCreature commented Nov 25, 2019 •

edited

Loading

FeepingCreature commented Nov 25, 2019 •

edited

Loading

FeepingCreature commented Nov 25, 2019 •

edited

Loading

FeepingCreature commented Nov 25, 2019 •

edited

Loading

FeepingCreature commented Nov 26, 2019 •

edited

Loading

FeepingCreature commented Nov 27, 2019 •

edited

Loading

FeepingCreature commented Nov 27, 2019 •

edited

Loading

FeepingCreature commented Nov 28, 2019 •

edited

Loading

FeepingCreature commented Dec 3, 2019 •

edited

Loading