Skip to content

add git (submodule) integrity check - #923

Merged
JukkaL merged 8 commits into
python:masterfrom
matthiaskramm:git-integrity-check
Oct 17, 2015
Merged

JukkaL merged 8 commits into
python:masterfrom
matthiaskramm:git-integrity-check

Conversation

@matthiaskramm

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@jhance

jhance commented Oct 13, 2015

Copy link
Copy Markdown
Collaborator

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

@matthiaskramm

Copy link
Copy Markdown
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'.
Comment thread mypy/git.py Outdated

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@JukkaL

JukkaL commented Oct 14, 2015

Copy link
Copy Markdown
Collaborator

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
Comment thread mypy/git.py Outdated

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

JukkaL commented Oct 15, 2015

Copy link
Copy Markdown
Collaborator

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