conda-build icon indicating copy to clipboard operation
conda-build copied to clipboard

Add source.git_recursive key to enable/disable recursive cloning of submodules

Open Chilipp opened this issue 4 years ago • 16 comments

closes #1210, #1733, #753

This PR adds something, that has already been discussed in PR #753 but stopped at one point. However, I think that there is still of interest (see #1733 for instance) and it is not difficult to implement.

This PR adds the source.git_recursive item to the conda build metadata (meta.yaml) that defaults to True and can be set to False in order to enable/disable the cloning of submodules when using source.git_url.

Commits:

  • https://github.com/conda/conda-build/commit/587202aa409643b632fc9a0fd8ded93cb693de0c adds the functionality
  • https://github.com/conda/conda-build/commit/843133b86ba1fb19731cac934e0f745d3c23e0a1 adds a test
  • https://github.com/conda/conda-build/commit/4b74d5b5561dcbdef8aaba1a3f2de21d1ca425f8 adds the docs
  • https://github.com/conda/conda-build/commit/e140ea4f027395db7766aeb9252adc15b9290f8d adds the news entry

Usage:

Disable the recursive cloning of submodules via

source:
  git_url: https://github.com/Chilipp/https://github.com/Chilipp/conda_build_test_recipe
  git_recursive: false

Chilipp avatar Mar 20 '20 19:03 Chilipp

We require contributors to sign our Contributor License Agreement, and we don't have one on file for @Chilipp. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature.

cla-bot[bot] avatar Mar 20 '20 19:03 cla-bot[bot]

We require contributors to sign our Contributor License Agreement

Alright, signed it.

One more comment that I forgot: In order to test the implemented changes (see https://github.com/conda/conda-build/pull/3910/commits/843133b86ba1fb19731cac934e0f745d3c23e0a1) I added a submodule to my fork of https://github.com/conda/conda_build_test_recipe at https://github.com/Chilipp/conda_build_test_recipe which can be found at https://github.com/Chilipp/conda_build_test_recipe_submodule. I leave it to you, the maintainers, how you want this to be implemented. I can

  • either transfer the ownership of this repository to the https://github.com/conda organization and make a PR for https://github.com/Chilipp/conda_build_test_recipe,
  • or you set it up by yourself the way you want

Chilipp avatar Mar 20 '20 20:03 Chilipp

Thank you so much for working on this. We'll get back to you on the repo ownership.

Cheers!

mingwandroid avatar Mar 21 '20 00:03 mingwandroid

We require contributors to sign our Contributor License Agreement, and we don't have one on file for @Chilipp. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature.

cla-bot[bot] avatar Mar 21 '20 16:03 cla-bot[bot]

We require contributors to sign our Contributor License Agreement, and we don't have one on file for @Chilipp. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature.

cla-bot[bot] avatar May 10 '20 20:05 cla-bot[bot]

Hey @mingwandroid,

as this is still open for more than a month now, are there any interests to merge this? If not, shall we close this PR?

Note that the CI errors were not caused by the new features implemented in this PR but rather by problems in the master branch of this repo. I also signed the CLA, but if there has been any problems with it, let me know and I'll sign it again.

Chilipp avatar May 10 '20 20:05 Chilipp

I am keen to get this looked at and resolved, yes, but I am also extremely busy, both with work on conda-build and other packages.

I did take a bit of a refresher look at this and have the following thoughts at present:

  1. @gmarkall's original reason for wanting a separate way to disable submodules was because when we mirror a repo, we don't also mirror the submodules (I think, correct me if I am wrong). I wonder if that problem can be addressed via: https://stackoverflow.com/a/31627058

  2. As @stuarteberg alluded it, I think, is that if it can be addressed as above, then the existing source/git_depth metadata value being 0 should be workable and would to me, be cleaner. @jakirkham mentioned that these are orthogonal, but I am not sure why a non-recursive clone cannot or should not be implemented as a clone of depth 0.

  3. Even if the implementation cannot be done via the SO link above, I do not see that introducing a new separate metadata variable for this instead of just handling this as a special case when depth is set to 0 is better. Just thinking about the documentation for this would make me kind of queasy vs writing: "A git_depth of zero means that no submodules are mirrorer or cloned."

I don't think they necessarily should be orthogonal from the users' perspective but am partake in discussion around that. I definitely want to get this addressed.

mingwandroid avatar May 11 '20 12:05 mingwandroid

  1. @gmarkall's original reason for wanting a separate way to disable submodules was because when we mirror a repo, we don't also mirror the submodules (I think, correct me if I am wrong). I wonder if that problem can be addressed via: https://stackoverflow.com/a/31627058

I can't recall many specifics, but I think the problem for me was that every build cloned the submodules from a remote machine rather than any local mirror.

  1. As @stuarteberg alluded it, I think, is that if it can be addressed as above, then the existing source/git_depth metadata value being 0 should be workable and would to me, be cleaner. @jakirkham mentioned that these are orthogonal, but I am not sure why a non-recursive clone cannot or should not be implemented as a clone of depth 0.

Setting it to 0 presumably has the same downside as setting it to 1? https://github.com/conda/conda-build/blob/master/docs/source/resources/define-metadata.rst#source-from-git - "The downside to setting it at 1 is that, unless the tag is on that specific commit, then you won't have that tag when you go to reference it in git_rev (for example). If your git_depth is insufficient to capture the tag in git_rev, you'll encounter an error. So in the example above, unless the 1.1.4 is the very head commit and the one that you're going to grab, you may encounter an error."

gmarkall avatar May 11 '20 13:05 gmarkall

Hey @mingwandroid,

glad you're still interested :) It's also not only about mirroring a local repository. When I want to build a conda recipe from a remote git repository, I do not want to necessarily clone all the submodules (that may only exist for test reasons and are not necessary for the build).

Within the current version of conda-build, all the git submodules are cloned, if you use source.git_url. Check the following

meta.yaml
package:
  name: dummy-git-test
  version: 0.0.1

source:
  git_url: https://github.com/Chilipp/conda_build_test_recipe.git

build:
  number: 1
  script: ls submodule/README.md

requirements:
  build:
    - python

It does succeed (i.e. the submodule is checked out). For instance, I often put large binary test-data files in a submodule to keep it separate from source code. This test-data is irrelevant for conda-build, but running conda build still clones it when specifying the git_url as it is linked to the source code via submodule.


Concerning the source.git_depth option. Not sure if I understand you correct but are you saying that setting source.git_depth: 0 means conda build does not clone the submodules? This would be (1) a strange behaviour to me for a config value such as git_depth, and (2) I also cannot confirm that this is true. The following recipe

meta.yaml
package:
  name: dummy-git-test
  version: 0.0.1

source:
  git_url: https://github.com/Chilipp/conda_build_test_recipe.git
  git_depth: 0

build:
  number: 1
  string: {{ GIT_BUILD_STR }}
  script: ls submodule/README.md

requirements:
  build:
    - python

still clones the submodule as well.

Chilipp avatar May 11 '20 13:05 Chilipp

Thanks @Chilipp, likely I am confusing two concepts! I would like to implement a git_submodule_depth thing here I think. Also when dealing with a local-filesystem path I do not think conda-build should bother with mirrors at all, or that should be configurable at least.

mingwandroid avatar May 12 '20 18:05 mingwandroid

Alright, thanks for the clarification @mingwandroid. Sure, that sounds reasonable. In this case, git_submodule_depth: -1 would be a recursive update, I guess. So we replace

https://github.com/conda/conda-build/blob/80272d374a952fbc346634802b135c473675520e/conda_build/source.py#L293-L295

with

        if git_submodule_depth == -1:
            check_call_env([git, 'submodule', 'update', '--init',
                            '--recursive'], cwd=checkout_dir, stdout=stdout, stderr=stderr)
        elif git_submodule_depth:
            check_call_env([git, 'submodule', 'update', '--init',
                            '--depth', git_submodule_depth], cwd=checkout_dir, stdout=stdout, stderr=stderr)

does this sound okay?

Chilipp avatar May 12 '20 21:05 Chilipp

Any update on this? :)

h-vetinari avatar Apr 10 '21 11:04 h-vetinari

Vote up for this feature

leshikus avatar Aug 10 '22 07:08 leshikus

Any progress on this PR? It would be a really useful feature.

adam-grofe avatar Apr 06 '23 16:04 adam-grofe

cc @jezdez

jakirkham avatar May 08 '23 19:05 jakirkham

Hi there, thank you for your contribution!

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further activity occurs.

If you would like this pull request to remain open please:

  1. Rebase and verify the changes still work
  2. Leave a comment with the current status

NOTE: If this pull request was closed prematurely, please leave a comment.

Thanks!

github-actions[bot] avatar May 08 '24 04:05 github-actions[bot]