Skip to content
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

Use "sha256sum" on Windows #1566

Merged
merged 4 commits into from Oct 10, 2016
Merged

Use "sha256sum" on Windows #1566

merged 4 commits into from Oct 10, 2016

Conversation

sschuberth
Copy link
Member

@sschuberth sschuberth commented Oct 8, 2016

Some changes around using "sha256sum" instead of "shasum" on Windows, in particular on MSYS2.

Integer checks are more clear for this kind of boolean variables, and the
string checks should have used a single "=" anyway.
Quoting [1] for some background information:

"MSYS2 consists of three subsystems and their corresponding package
repositories, msys2, mingw32, and mingw64.

The mingw subsystems provide native Windows programs and are the main
focus of the project. These programs are built to co-operate well with
other Windows programs, independently of the other subsystems.

The msys2 subsystem provides an emulated mostly-POSIX-compliant
environment for building software, package management, and shell
scripting. These programs live in a virtual single-root filesystem (the
root is the MSYS2 installation directory). Some effort is made to have the
programs work well with native Windows programs, but it's not seamless.

[...]

Every subsystem has an associated "shell", which is essentially a set of
environment variables that allow the subsystems to co-operate properly.
These shells can be invoked using scripts in the MSYS2 installation
directory or shortcuts in the Start menu. The scripts set the MSYSTEM
variable and start a terminal emulator with bash. Bash in turn sources
/etc/profile which sets the environment depending on the value of
MSYSTEM."

[1] https://sourceforge.net/p/msys2/wiki/MSYS2%20introduction/
Copy link
Contributor

@ttaylorr ttaylorr left a comment

Choose a reason for hiding this comment

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

Just the one comment, otherwise this looks great. Excited to get this merged in and running on Windows!

fi | cut -f 1 -d " "
}

calc_oid_file() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that there wasn't documentation on calc_oid before, but adding documentation to both of these functions would be incredibly helpful 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but could you be a bit more clear on what to document? I mean, should I basically just use the commit message and document why to use sha256sum on Windows, or do you want to have the pipe of commands itself documented?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, let's just document what these functions take in their argument and that they work on both macOS and Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Like so?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect!

Copy link
Contributor

@ttaylorr ttaylorr left a comment

Choose a reason for hiding this comment

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

LGTM.

/cc-ing @larsxschneider for a final approval.

sha256sum "$1"
else
shasum -a 256 "$1"
fi | cut -f 1 -d " "
Copy link
Member

Choose a reason for hiding this comment

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

I guess there is no reasonable nice way to have the decision between sha256sum and shasum once in the code? Maybe a sha256 function? I guess "eval" is too evil?!

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I just introduced a test environment variable for that. Let's see if that works on CI.

@larsxschneider
Copy link
Member

@sschuberth The unit tests run clean on my Win7 box but the integration tests don't work. Should they?

$ ./script/integration
/c/path/to/my/workspace/src/github.com/github/git-lfs/test/remote/url still exists!
Confirm other tests are done, and run:
  $ curl http://127.0.0.1:51043/shutdown
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (7) Failed to connect to 127.0.0.1 port 51043: Connection refused

Plus, I can't find any output of the unit or integration tests in the appveyor logs. Should they be there? Maybe I am not looking at the right places - I am not familiar with appveyor.

Thanks for working on the Windows CI check! 👍

"shasum" is a Perl script. Pure MSYS2, in contrast to Git for Windows,
does not come with Perl by default, so fall back to using "sha256sum" on
Windows in general.
@technoweenie
Copy link
Contributor

@larsxschneider: that's an indication of a previous unclean test run. Just delete /c/path/to/my/workspace/src/github.com/github/git-lfs/test/remote/url and try it again.

@sschuberth
Copy link
Member Author

The unit tests run clean on my Win7 box but the integration tests don't work. Should they?

Yes, and they do on my machine as well as on CI.

Plus, I can't find any output of the unit or integration tests in the appveyor logs. Should they be there?

Yes, see the same link as above. However, the test results do not show up on the tests tab yet as AppVeyor only supports a limited set of test result formats. We would first need to write some simple conversion script, but I wanted to leave that for later.

@larsxschneider
Copy link
Member

Of course... the URL file. I should have read the error properly. Sorry.

All but two integration tests pass on my local Windows box:

test: attempt private access without credential helper ...         FAILED
test: push --all (no ref args) ...                                 FAILED

However, that's probably related to some of my setup. If they pass on the CI that this is good enough I think.

LGTM 👍

@ttaylorr ttaylorr merged commit bcc584c into git-lfs:master Oct 10, 2016
@ttaylorr
Copy link
Contributor

Let's do it!

@sschuberth sschuberth deleted the windows-sha256sum branch October 10, 2016 18:01
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.

None yet

4 participants