Fetch strategies: new global option no_cache, new git options full_depth and all_branches. #12122

greenc-FNAL · 2019-07-24T20:25:06Z

Fetch strategies: new global option no_cache, new git option get_full_repo.

All fetch strategies now accept the Boolean version keyword option no_cache in order to allow per-version control of cache-ability.
New git-specific version keyword option get_full_repo (Boolean). When true, disables the default --depth 1 and --single-branch optimizations that are applied if supported by the git version and (in the former case) transport protocol.
The try / catch blog attempting --depth 1 and retrying on failure has been removed in favor of more accurately ascertaining when the --depth option should work based on git version and protocol choice. Any failure is now treated as a real problem, and the clone is only attempted once.
Test improvements:
mock_git_repository.checks[type_of_test].args['git'] is now specified as the URL (with leading file://) in order to avoid complaints when using --depth.
New type_of_test tag-branch.
mock_git_repository now provides git_exe.
Improved the action of the git_version fixture, which was previously hard-wired.
New tests of --single-branch and --depth 1 behavior.
Add documentation of new options to the packaging guide.

lib/spack/docs/packaging_guide.rst

tldahlgren · 2019-08-01T21:01:59Z

lib/spack/spack/fetch_strategy.py

        # The stage is initialized late, so that fetch strategies can be
        # constructed at package construction time.  This is where things
        # will be fetched.
        self.stage = None
+        # Possibly disable caching for this strategy.
+        self._no_cache_opt = kwargs.pop('no_cache', False)


Is there a reason you want to remove no_cache from kwargs using pop (versus get)?

Also, given the checks below tend toward (not self._no_cache_opt), using something more positive (e.g., cache, enable_caching) would simplify the checks.

I popped it from kwargs since the option has now been dealt with and does not need to be in kwargs as seen by sub-classes: just separation of concerns, basically.

Per your second point: I have a natural inclination to avoid options that are true by default. I could certainly see the attribute being something like self.cache_enabled, but I'd like to continue to have the option name reflect what happens when it is set to the non-default True case. Does this make sense to you?

I can see your reasoning in both cases and neither point is a blocker.

I am curious about the basis for your natural inclination simply for my own edification.

I'm not sure I would claim my natural inclination had a conscious, "basis"—or was even entirely rational—but I guess that when I'm adding an option to something, I like specifying the option to be an affirmative act i.e. I'm specifying the option explicitly because I want to change something about how it (function, command, etc.) behaves. I would therefore like the name of the option to reflect what I want to happen when I specify it, which (in my mind) equates to wanting the option to be True when I specify it.

I hope that makes sense to you. It's a bit of a post hoc rationalization since I can't actually put my finger on exactly when or why I started feeling this way.

I can appreciate your perspective. Thanks for sharing.

lib/spack/spack/fetch_strategy.py

lib/spack/spack/test/git_fetch.py

tldahlgren

LGTM. I do suggest the following changes:

Clearer if explain (in docs) what the new options mean when True first
Add the missing underscore at line 329 of fetch_strategy.py

greenc-FNAL · 2019-08-02T18:06:05Z

Clearer explanation of new options in terms of behavior when True.
self._no_cache_opt -> self._cache_enabled; fix missing underscore(s) along the way.

tldahlgren

LGTM. Thanks for the changes.

becker33 · 2019-08-02T21:45:25Z

@chissg Could you expand a bit on what the use cases for these options are? I'm not sure I quite get it yet.

For the no-cache option I understand we shouldn't cache versions that are moving heads of branches, but what are the others for?

greenc-FNAL · 2019-08-05T19:29:27Z

@becker33 For the no_cache option, there's also the—rarer, admittedly—occasional case where a tagged version is prone to having its tag moved—a 'snapshot' tag, say.

For all_branches and full_depth, these are intended for mainly cases where one is intending to develop the checked-out source rather than just build it as a one-off. This may be useful for spack diy, for instance, but is already in use by spack dev, where these options are injected into the source download operation in order to ensure full histories for code to be developed.

scheibelp

I have a few minor (hopefully) requests and one possibly larger request to merge --full-depth and --all-branches (if that wouldn't cause problems for your use cases).

Also regarding the PR description:

The try / catch block attempting --depth 1 and retrying on failure has been removed in favor of more accurately ascertaining when the --depth option should work based on git version and protocol choice. Any failure is now treated as a real problem, and the clone is only attempted once.

To be clear, is this to avoid cases where a temporary connectivity failure changes Spack's behavior? If so, I'm definitely a fan of this.

lib/spack/spack/fetch_strategy.py

lib/spack/docs/packaging_guide.rst

lib/spack/spack/fetch_strategy.py

greenc-FNAL · 2019-08-15T19:26:14Z

I have a few minor (hopefully) requests and one possibly larger request to merge --full-depth and --all-branches (if that wouldn't cause problems for your use cases).

@scheibelp, I believe I have addressed your outstanding issues with 47f738b. Please let me know if you have any further comments on the latest commit.

Also regarding the PR description:

The try / catch block attempting --depth 1 and retrying on failure has been removed in favor of more accurately ascertaining when the --depth option should work based on git version and protocol choice. Any failure is now treated as a real problem, and the clone is only attempted once.

To be clear, is this to avoid cases where a temporary connectivity failure changes Spack's behavior? If so, I'm definitely a fan of this.

Yes, that's exactly what this was for.

scheibelp

This looks nearly there but I have a question and a couple minor comments. Probably the most work would be pulling out the emacs directives into their own commit and then rebasing the PR into a set of clean commits (normally that doesn't matter since I would squash the commits but here I think a separate commit for the Emacs directives would be good).

lib/spack/spack/test/conftest.py

scheibelp · 2019-08-16T20:47:12Z

lib/spack/docs/packaging_guide.rst

@@ -4280,3 +4289,8 @@ Autotools-based packages would be easy (and should be done by a
 developer who actively uses Autotools).  Packages that use
 non-standard build systems can gain ``setup`` functionality by
 subclassing ``StagedPackage`` directly.
+
+.. Emacs local variables


This looks good but it would be ideal to move this to a different commit; in that case if you could rebase the PR that would be great (e.g. to remove flake commits).

Done. Does the commit history match what you want, now?

Sorry no I was not clear, I think this should be collapsed into 2 commits: one which contains the emacs directives and one that contains all the other changes. The commit messages would have to be "clean" as well (since I plan to merge the commits as-is into develop), e.g. avoid messages like "address review comments from X".

@scheibelp How about now?

Yes this organization is excellent

BUT I think the first commit message has at least one stale note:

New git-specific version keyword options full_depth and all_branches (both Boolean). The former disables the --depth 1 optimization and the latter the --single-branch optimization.

I think as of now there is only get_full_repo

Once that is addressed, this will be set

@scheibelp Done, I think. Let me know if there's anything else.

lib/spack/spack/test/git_fetch.py

…ull_repo`. * All fetch strategies now accept the Boolean version keyword option `no_cache` in order to allow per-version control of cache-ability. * New git-specific version keyword option `get_full_repo` (Boolean). When true, disables the default `--depth 1` and `--single-branch` optimizations that are applied if supported by the git version and (in the former case) transport protocol. * The try / catch blog attempting `--depth 1` and retrying on failure has been removed in favor of more accurately ascertaining when the `--depth` option should work based on git version and protocol choice. Any failure is now treated as a real problem, and the clone is only attempted once. * Test improvements: * `mock_git_repository.checks[type_of_test].args['git']` is now specified as the URL (with leading `file://`) in order to avoid complaints when using `--depth`. * New type_of_test `tag-branch`. * mock_git_repository now provides `git_exe`. * Improved the action of the `git_version` fixture, which was previously hard-wired. * New tests of `--single-branch` and `--depth 1` behavior. * Add documentation of new options to the packaging guide.

scheibelp · 2019-08-22T01:21:12Z

Thanks!

greenc-FNAL added feature fetching git labels Jul 24, 2019

greenc-FNAL closed this Jul 25, 2019

greenc-FNAL reopened this Jul 25, 2019

greenc-FNAL requested review from tldahlgren and scheibelp July 25, 2019 16:10

tldahlgren reviewed Aug 1, 2019

View reviewed changes

lib/spack/docs/packaging_guide.rst Outdated Show resolved Hide resolved

tldahlgren reviewed Aug 1, 2019

View reviewed changes

lib/spack/spack/fetch_strategy.py Outdated Show resolved Hide resolved

tldahlgren reviewed Aug 1, 2019

View reviewed changes

lib/spack/spack/fetch_strategy.py Outdated Show resolved Hide resolved

tldahlgren reviewed Aug 1, 2019

View reviewed changes

lib/spack/spack/test/git_fetch.py Show resolved Hide resolved

tldahlgren requested changes Aug 1, 2019

View reviewed changes

greenc-FNAL force-pushed the feature/fetch-strategy-options branch from e27c4ea to 5aeb9e5 Compare August 2, 2019 18:05

tldahlgren self-requested a review August 2, 2019 21:35

tldahlgren approved these changes Aug 2, 2019

View reviewed changes

scheibelp requested changes Aug 13, 2019

View reviewed changes

lib/spack/spack/fetch_strategy.py Outdated Show resolved Hide resolved

lib/spack/docs/packaging_guide.rst Outdated Show resolved Hide resolved

lib/spack/docs/packaging_guide.rst Outdated Show resolved Hide resolved

lib/spack/spack/fetch_strategy.py Outdated Show resolved Hide resolved

greenc-FNAL force-pushed the feature/fetch-strategy-options branch from 5aeb9e5 to 47f738b Compare August 15, 2019 19:20

greenc-FNAL force-pushed the feature/fetch-strategy-options branch from cdf2e1a to 880d445 Compare August 16, 2019 12:37

scheibelp requested changes Aug 16, 2019

View reviewed changes

greenc-FNAL force-pushed the feature/fetch-strategy-options branch 2 times, most recently from cadc6fd to c024d8d Compare August 19, 2019 14:38

greenc-FNAL added 2 commits August 20, 2019 14:39

Emacs directives for fill-column setting.

f9cf574

greenc-FNAL force-pushed the feature/fetch-strategy-options branch from c024d8d to f9cf574 Compare August 20, 2019 19:40

scheibelp approved these changes Aug 22, 2019

View reviewed changes

scheibelp merged commit 2499c04 into spack:develop Aug 22, 2019

greenc-FNAL deleted the feature/fetch-strategy-options branch August 22, 2019 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch strategies: new global option no_cache, new git options full_depth and all_branches. #12122

Fetch strategies: new global option no_cache, new git options full_depth and all_branches. #12122

greenc-FNAL commented Jul 24, 2019 •

edited

tldahlgren Aug 1, 2019 •

edited

greenc-FNAL Aug 2, 2019

tldahlgren Aug 2, 2019

greenc-FNAL Aug 2, 2019

tldahlgren Aug 2, 2019

tldahlgren left a comment •

edited

greenc-FNAL commented Aug 2, 2019

tldahlgren left a comment

becker33 commented Aug 2, 2019 •

edited

greenc-FNAL commented Aug 5, 2019

scheibelp left a comment

greenc-FNAL commented Aug 15, 2019

scheibelp left a comment

scheibelp Aug 16, 2019

greenc-FNAL Aug 16, 2019

scheibelp Aug 17, 2019

greenc-FNAL Aug 19, 2019

scheibelp Aug 20, 2019

greenc-FNAL Aug 21, 2019

scheibelp commented Aug 22, 2019

Fetch strategies: new global option no_cache, new git options full_depth and all_branches. #12122

Fetch strategies: new global option no_cache, new git options full_depth and all_branches. #12122

Conversation

greenc-FNAL commented Jul 24, 2019 • edited

tldahlgren Aug 1, 2019 • edited

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

tldahlgren left a comment • edited

Choose a reason for hiding this comment

greenc-FNAL commented Aug 2, 2019

tldahlgren left a comment

Choose a reason for hiding this comment

becker33 commented Aug 2, 2019 • edited

greenc-FNAL commented Aug 5, 2019

scheibelp left a comment

Choose a reason for hiding this comment

greenc-FNAL commented Aug 15, 2019

scheibelp 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

scheibelp commented Aug 22, 2019

greenc-FNAL commented Jul 24, 2019 •

edited

tldahlgren Aug 1, 2019 •

edited

tldahlgren left a comment •

edited

becker33 commented Aug 2, 2019 •

edited