Skip to content

add git (submodule) integrity check #923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Oct 17, 2015

Conversation

matthiaskramm
Copy link
Contributor

If mypy is a git repository and has submodules, check that the submodules are initialized and up-to-date (and abort if not) in runtests.py, scripts/mypy, setup.py. Also check if they're dirty, and output a warning if so.
New option --dirty-stubs for runtests.py + scripts/mypy, for bypassing said checks.

This is a standalone PR. It doesn't depend on #884, or the other way around.
So you can merge them in any order.

If mypy is a git repository, check that the submodules are up-to-date (and
abort if not) in runtests.py, scripts/mypy, setup.py.
New option --dirty-stubs for runtests.py + scripts/mypy, for bypassing said
check.
@matthiaskramm
Copy link
Contributor Author

Good catch. Will use subprocess.check_output(cwd=...).

@jhance
Copy link
Collaborator

jhance commented Oct 13, 2015

I'd rather just have it warn the user rather than fail and requiring a bypass flag.

@matthiaskramm
Copy link
Contributor Author

I'd rather just have it warn the user rather than fail and requiring a bypass flag.

It's easy to change. We'll see what developers want. Jukka (in #882) seemed to prefer for it to fail.
(Keep in mind, if you're running with a wrong version of typeshed, you might get a much more bizarre failure later, with the submodule warning long scrolled out of your window)

'git submodule status' returns the current revision, not the revision in the
superproject's index. Do an explicit retrieval of the latter using 'git ls-files'.


def is_git_repo(dir: str) -> bool:
"""Is the passed directory version controlled with git?"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like someone..."expelled" a directory or something. :) What about "the given directory"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 14, 2015

Thanks for implementing this! Looks good. I'll play around this with together with the submodule PR but I'll try to get this in as soon as possible to avoid the manual copying of changes between mypy and typeshed.

I'd rather just have it warn the user rather than fail and requiring a bypass flag.

Let's start with failing. As Matthias mentioned, it's possible to miss the error, and if this turns out to be a burden then we can always reconsider it. People get quickly desensitized to errors and warnings.

Only abort if the submodule is behind. Warn even if dirty_stubs is True.

I'd prefer to always abort if the submodule state is dirty. It just feels more consistent that way to me, and using a too recent submodule could cause problems. For example, it may use features that aren't supported by an older revision of mypy.

@JukkaL JukkaL mentioned this pull request Oct 14, 2015
print("Submodule '{}' not initialized.".format(name), file=sys.stderr)
print("Please run:", file=sys.stderr)
print(" cd {}".format(pipes.quote(dir)), file=sys.stderr)
print(" git submodule init {}".format(name), file=sys.stderr)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we ask the user to run git submodule update ... here as well, as otherwise the next try will also fail as the repo isn't cloned?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be nicer to have all commands on a single line (as git submodule init ... && git submodule update ...) for easier copy-paste (but only on a POSIX system). I guess it's safe to assume a sufficiently bash-like shell?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Also, changed "git submodule init" to "git submodule update --init", which does both things at once.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 15, 2015

Thanks for the updates! I'll look at this again later this week when I have more time.

@JukkaL JukkaL merged commit 5ad7a45 into python:master Oct 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants