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

Download release file instead of cloning repo in go-template (continued from #179) #180

Merged
merged 31 commits into from Aug 27, 2017

Conversation

Projects
None yet
2 participants
@mbland
Owner

mbland commented Aug 25, 2017

Closes #179.

I've taken @elpaquete's fine work from #179 and refactored it a bit to:

  • rebase on master (after I remembered to push the v1.5.0 commit just today—d'oh!)
  • fix a few bugs
  • add a few tests
  • clean up the formatting
  • restore the ability to test it without a network connection
  • add the capability to use wget and fetch as well.

I still need to test this on all the platforms (I think I might need to add tweaks for MSYS2), but I hope to have it in by tomorrow and cut v1.6.0 right away.

@elpaquete Note that I found a way to get rid of curl | tar and set -o pipefail, namely by using a command substitution and checking that the expected output is present.

Juan Saavedra and others added some commits Jun 26, 2017

go-template: Update PIPEFAIL_BACKUP usage
Rather than use a bare string, we capture the output in an array.
tests/template: Add assert_go_core_unpacked
This provides a better error than a raw `[[ -f ... ]]`.
go-template: Test in TEST_GO_ROOTDIR, fix quoting
Don't know why the test wasn't originally written using
`TEST_GO_ROOTDIR`. As intended, it exposed some missing quotes from
variables in `go-template` introduced in #179.

I've removed several instances of the following comment, as I'm not sure
it applies anymore. If it does, I'll make sure to restore it as needed:

  # Use `.*/scripts/go-script-bash` to account for the fact that `git clone` on
  # MSYS2 will output `C:/Users/<user>/AppData/Local/Temp/` in place of `/tmp`.
go-template: Use variable to shorten commands
Also wraps a couple of lines that were previously longer than 80
columns.
tests/template: Remove PATH assignments
These aren't necessary, as `stub_program_in_path` will update `PATH`
automatically.
go-template: Eliminate `curl | tar`
By using a process substitution to provide input to `tar`, we eliminate
a subshell and the need for `set -o pipefail`.
go-template: Make download, git clone top-level
Seems cleaner and easier to follow to give both operations descriptive
names and allow them to reside at the top level.
go-template: Use command -v to detect curl, tar
This prevents new processes from spawning just to detect whether a
program is present.

The new `create_forwarding_script` helper in `tests/template.bats`
ensured that the tests continued to pass before and after this change.
`create_forwarding_script` will move to `lib/bats/helpers` in a future
commit.
go-template: Update missing curl, tar messages
In addition to being more specific, the errors are now printed to
standard error.
tests/template: Use variable to shorten lines
Now all of the lines are back down to 80 characters or less.
tests/template: Hoist assignments to top level
Defining these variables at the top level keeps all the environment
variables defined in one consistent location, and makes them available
to suite names as well.
tests/template: Restore GO_SCRIPT_BASH_REPO_URL
This had been hardcoded to the actual repo value in #179. The
`create_fake_tarball_if_not_using_real_url` helper function and
`TEST_ARCHIVE_URL` environment variable enable the test to run isolated
from the network.

Also tweaks `GO_SCRIPTS_DIR` to remove unnecessary braces.
template: Improve download_go_script_bash_tarball
Got rid of `GO_SCRIPT_BASH_VERSION_NUMBER`, since it was only used in
this function. Elided some of the `if` conditions into a single
`if/elif` chain. Made sure that the `mkdir -p` tries to create the full
parent of `GO_SCRIPT_BASH_CORE_DIR`, not just `GO_SCRIPTS_DIR`.
go-template: Download using curl, wget, or fetch
Attempts to be flexible as the `./go get file` command. The
`run_with_download_program` test helper creates a script that runs `cat`
on a local tarball to simulate downloads using each tool.

@mbland mbland added the enhancement label Aug 25, 2017

@mbland mbland self-assigned this Aug 25, 2017

mbland added some commits Aug 25, 2017

tests/template: Fix Linux errors with missing gzip
The `download uses ...` tests all restrict the avaiable programs to a
very small subset via forwarding scripts. Apparently GNU tar, unlike BSD
tar, calls out to the `gzip` program rather than linking in the library.
So this change adds a forwarding script for `gzip` to fix this
particular breakage:

* https://travis-ci.org/mbland/go-script-bash/jobs/268538634
tests/template: Update nonexistent repo test
As I'd already done with the 'fail to download a nonexistent version'
test case, I've updated the 'fail to download a nonexistent repo' test
case to use a series of `assert_output_matches` assertions rather than
checking the literal output with `assert_failure`. This avoids the
problem whereby tool+platform-specific error messages can cause the
tests to fail:

* https://travis-ci.org/mbland/go-script-bash/jobs/268538634
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 25, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 57e7356 on go-template-download into 779f544 on master.

coveralls commented Aug 25, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 57e7356 on go-template-download into 779f544 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 25, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 238fcda on go-template-download into 779f544 on master.

coveralls commented Aug 25, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 238fcda on go-template-download into 779f544 on master.

mbland added some commits Aug 26, 2017

go-template: Minor formatting tweaks
This is in preparation for using `cp` instead of `wget` for `file://`
urls, since `wget` only supports HTTP, HTTPS, and FTP.
go-template: Use `cat` with `file://` URLs
This is because `wget` only supports HTTP, HTTPS, and FTP, as I
discovered when running the tests on Alpine Linux without cURL
installed.

Also, the BusyBox version of `tar` that comes with Alpine doesn't work
out of the box; the GNU version must be installed via `apk add tar`. Not
sure it's worth adding a check to `go-template` or not.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 26, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 0784a04 on go-template-download into 779f544 on master.

coveralls commented Aug 26, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 0784a04 on go-template-download into 779f544 on master.

tests/template: Add PATH to forwarding scripts
This fixes test failures for the "fail to find download program uses git
clone" and the "fail to find tar uses git clone" test cases on MSYS2 and
Cygwin, due to the fact that helper programs couldn't be found with the
`PATH`. This resulted in errors such as:

  C:/msys64/usr/lib/git-core/git-upload-pack.exe: error while loading
  shared libraries: ?: cannot open shared object file: No such file or
  directory
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 26, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling d27dc12 on go-template-download into 779f544 on master.

coveralls commented Aug 26, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling d27dc12 on go-template-download into 779f544 on master.

go-template: Use `cygpath` for Windows portability
Cygwin and MSYS2 environments both contain the `cygpath` utility for
translating between Unix and native Windows file paths. `cygpath -m` is
just what's needed when passing file paths and URLs to native binary
programs such as `curl` and `git`.

This commit adds the `windows_native_path` helper function, which I'll
eventually extract into a `lib/` module at some point. It also adds
logic to ensure `GO_SCRIPT_BASH_DOWNLOAD_URL` begins with a protocol,
adds a test case to validate this, removes redundant comments and
assertions from several `git clone` test cases, and updates the
`create_forwarding_script` helper to only generate a script when a
program is present on the system.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 26, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 2f6aeca on go-template-download into 779f544 on master.

coveralls commented Aug 26, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 2f6aeca on go-template-download into 779f544 on master.

go-template: Only use `cygpath` on Git for Windows
As it turns out, regular MSYS2 and Cygwin do not work with with file
paths modified with `cygpath -m`. This change tries to ensure only Git
for Windows is affected by checking the `EXEPATH` variable.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling f4f5fd7 on go-template-download into 779f544 on master.

coveralls commented Aug 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling f4f5fd7 on go-template-download into 779f544 on master.

tests/template: Fix `git_for_windows_native_path`
Turns out I made a mistake in the previous commit in adapting the
conditional to use `EXEPATH` instead of `command -v cygpath`, as the
tests were still broken on regular MSYS2 and Cygwin. This change
resolves the breakage and continues to work on Git for Windows.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 757cf46 on go-template-download into 779f544 on master.

coveralls commented Aug 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 757cf46 on go-template-download into 779f544 on master.

go-template: Use `git` to detect Git for Windows
Turns out this is the most reliable and robust way, as `EXEPATH` won't
always be set or set with 'Git' as its final component.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling fc55a10 on go-template-download into 779f544 on master.

coveralls commented Aug 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling fc55a10 on go-template-download into 779f544 on master.

tests/template: Make scripts for curl, wget, fetch
Turns out the 'fail to find tar uses git clone' test would fail on
systems that don't have `curl` installed, since
`create_forwarding_script` now only creates a script if a program is
present. This fixes the breakage by trying to create a forwarding script
for all three programs; at least one should be guaranteed to be present.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 96af8f4 on go-template-download into 779f544 on master.

coveralls commented Aug 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 94.605% when pulling 96af8f4 on go-template-download into 779f544 on master.

@mbland

This comment has been minimized.

Show comment
Hide comment
@mbland

mbland Aug 27, 2017

Owner

Wow, it literally took me all day to get things smoothed out across platforms. Specifically, I came to realize how particular the Git for Windows environment is, even compared to plain MSYS2 and Cygwin. But now I'm armed with insight from that experience (including my discovery of the cygpath utility), and all the tests pass everywhere.

Thanks for the contribution, @elpaquete! Though since it's almost 9pm and I haven't had dinner yet, I think I'll cut v1.6.0 tomorrow. 🙂

Owner

mbland commented Aug 27, 2017

Wow, it literally took me all day to get things smoothed out across platforms. Specifically, I came to realize how particular the Git for Windows environment is, even compared to plain MSYS2 and Cygwin. But now I'm armed with insight from that experience (including my discovery of the cygpath utility), and all the tests pass everywhere.

Thanks for the contribution, @elpaquete! Though since it's almost 9pm and I haven't had dinner yet, I think I'll cut v1.6.0 tomorrow. 🙂

@mbland mbland merged commit de7ce6c into master Aug 27, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 94.605%
Details

@mbland mbland deleted the go-template-download branch Aug 27, 2017

@mbland mbland added this to Done in v1.7.0 Sep 13, 2017

@mbland mbland removed this from Done in v1.7.0 Sep 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment