Doing this shouldn't cause any problems, and will remove some initial WTF for the uninitiated when cloning repos using submodules.

Comments

tizzo’s picture

I think this is a good idea, we're using submodules and while we need people to understand submodules that change often (they'll need to `git submodule sync ; git submodule update --init` after pulling to use submodules extensively, this lowers the barrier and would help with things like symfony components that change rarely.

eliza411’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

rfay’s picture

Status: Closed (fixed) » Needs work

Sorry to be a nay-sayer, and feel free to close this out again if you disagree

But:

1. What would be the use case for submodules in a Drupal project? I assume that any project contains only its own code.

2. Do we think it's OK for a "git clone" on drupal.org to check out code from a completely unrelated git repository (could checkout submodules from git.randyfay.com just fine with this technique). I don't really approve.

Sorry to come in after the fact.

helmo’s picture

I think I agree with rfay on this.

And where tizzo mentions symphony, I don't see any use of submodules in the current 8.x branch

eliza411’s picture

I haven't seen any support requests before this change or after (which doesn't mean they don't happen in IRC, etc.), so I don't have any particular opinion about which way this is written. I might if I had some idea how prevalent the use of submodules is on d.o.

I do know it's not in new contributors' best interest to change the instructions back and forth a bunch so I'd like to understand better before I change it again.

I'll bring it up on our next Git call and we can settle on one or the other next week.

tizzo’s picture

The use case was the scotch project (hence EclipseGc making the initial request). I imagine one of rfay's concerns is potential licensing issues and I'll bet our packaging scripts wouldn't work with submodules at all. Any project that takes this route will need to change things (in a way that could probably break pulls for people with existing checkouts) before packaging releases.

Upon reflection this change does imply that we are supporting submodules which we currently are not. We allow them but d.o doesn't really "support them". I like submodules but most people find them baffling. We could explicitly block them with a post-receive hook (i.e. reject your commit if it contains submodules) or we could allow them in the build scripts.

I'd like to see us have a clear answer here. Do we allow submodules partly (no infrastructure supports them but we don't prevent them), not at all (reject commits containing them) or completely (pull in the submods when building archives)? The answer should dictate what we do in this case.

@rfay If we allow install profiles to pull in external libs, why not allow submodules external repos?

rfay’s picture

@tizzo, I guess my understanding of profiles is that they build something that expects inclusion of specific external assets. In that case the package management system is doing it.

But if random submodules are supported, I can make a git repo which when cloned will have completely different behavior from the Drupal-hosted code, and --recursive makes that implicit; it doesn't ask the user or rely on the user's sense or anything. I'd be surprised if the security team would be OK with this. I have to admit that git users are probably not good targets for exploits, though. But this one step certainly makes it easier.

greggles’s picture

@rfay If we allow install profiles to pull in external libs, why not allow submodules external repos?

We do allow that, but the libs are limited to those that are approved, there's a review process, and it's revisioned. When someone downloads the tarball, especially of a profile, there is an expectation that it will very likely include external resources but those will come from an approved list. When someone clones a git repo I don't think there's an expectation that they might get code from some random place.

If scotch does need to do something like this then I think it should be via some other mechanism (e.g. make a scotch profile and let's get approval for the necessary libraries).

EclipseGc’s picture

OK, so I don't explicitly need this any longer, BUT my use case was other drupal repos. Similarly Crell did this initially with the Symfony code base he needed to pull into his core 8.x branch. If I had thought that bringing this up would remove the possibility of using submodules at all I'd have just let users suffer, because submodules are super useful... even if we have a whitelist of stuff we'll use. Bare minimum I suggest we continue to allow submodules against other drupal code, and likewise we should probably consult Crell about other things in the Symfony/Doctrine/Etc world we're likely to use. I understand randy's concern, but I think it's a little over stated. Chances are there are a relatively small number of repos out there even using submodules yet, and as randy well knows (since he's the person who taught sub modules to me) they're confusing for most users... but ridiculously useful.

TL:DR

Can we come up with a white list of submodule domains we're willing to continue working with? It's too useful to lose.

Eclipse

greggles’s picture

Can we come up with a white list of submodule domains we're willing to continue working with? It's too useful to lose.

Sure?

Let's start with drupal.org and nothing else :)

rfay’s picture

This issue is about promoting the use of git clone --recursive, right? How are we going to control git's behavior?

I don't even think that we should do anything about submodules; we should just accept them as they are. We just shouldn't encourage people to copy and paste instructions they may not understand that could result in unintended consequences.

tizzo’s picture

"As they are" meaning you can use them but need to change gears if you ever want to make a release that will be packaged, correct? I'd be fine with that.

I think that puts them in the "officially unsupported but allowed" category.

As for rfay's question in #13 I think if we wanted to actually prohibit submodules we could use introspection of the commits in the hooks to reject commits containing them from being pushed to d.o but I'd need to verify that.

ergonlogic’s picture

Unless support for git submodules is planned, I suggest pulling the '--recursive' from the documentation. There is no other purpose for this option, so it strongly implies that sub-modules will be packed with releases. It should also be documented in the Git docs for maintainers. As it stands, I was just stung by this, and have a broken release, as a result.

I fully understand the licensing concerns of including arbitrary 3rd-party code. It would, however, be nice to be able to include d.o sources, at least. Of course, I'm definitely a corner case, including Puppet modules for use with Vagrant through a Drush extension.

ergonlogic’s picture

I've updated the documentation on the following pages to point out that Git submodules are not packaged with releases:

ergonlogic’s picture

Title: Add --recursive to git clone text » Remove --recursive from git clone text on Drupal.org's version control tab
Category: feature » bug
Priority: Normal » Major

Sorry to keep harping on this, but this breaks developer expectations significantly, imo. Debugging issues reported by users that have downloaded a package is made significantly more difficult. I think it's normal to assume, as a developer, that the packaged version would match what's in the git repo (at a specified release tag, &c.) So, I'm upgrading this to a major bug report.

Unfortunately, this really isn't a problem for 'Project Git instructions', but rather the fork used on d.o, as documented here: http://drupal.org/node/1134632. I'm tempted to move this issue over to http://drupal.org/project/drupalorg, except that I don't see aforementioned fork in there...

Also, there are a number of tools to work around the lack of Git submodules, when you want to include external code. In my case, it was a combination of Composer and librarian-puppet. I'd be happy to draft up a doc page on this, assuming I'm not way out in left field here. I'd think somewhere under Best practices for creating and maintaining projects, with links to it from a couple of other spots.

helmo’s picture

Assigned: Unassigned » eliza411

I agree that the --recursive text is doing more harm then good.

+1 to remove it.

eliza411’s picture

Status: Needs work » Fixed

I've removed --recursive from the database configuration settings.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xhtmlwebdesign’s picture

Can not post specific currency to paypal.