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

Cleanup build script #1245

Merged
merged 3 commits into from
Nov 14, 2019
Merged

Cleanup build script #1245

merged 3 commits into from
Nov 14, 2019

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Nov 14, 2019

Build release script assumes GNU toolchains to make it works, thus running the script under Mac OSX fails. There're two problems:

  • date command invocation using GNU specific parameters.
  • Incorrect handling of checksum command invocation.

To fix them:

  • Use common parameters of both GNU date and BSD date, also use a specific time format form, instead of relying on --iso-8601 GNU option. This will change the timezone offset format from +00:00 to +0000.
  • Use an array to handle checksum command invocation.

While at it, also add -trimpath to all go build and go install command.

@cuonglm cuonglm added this to the v0.26.0 milestone Nov 14, 2019
@codecov-io
Copy link

codecov-io commented Nov 14, 2019

Codecov Report

Merging #1245 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
- Coverage    75.3%   75.28%   -0.02%     
==========================================
  Files         149      149              
  Lines       10841    10841              
==========================================
- Hits         8164     8162       -2     
- Misses       2210     2212       +2     
  Partials      467      467
Impacted Files Coverage Δ
stats/cloud/collector.go 76.99% <0%> (-0.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f12764...83a5337. Read the comment docs.

build-release.sh Outdated Show resolved Hide resolved
build-release.sh Show resolved Hide resolved
build-release.sh Outdated Show resolved Hide resolved
build-release.sh Show resolved Hide resolved
@cuonglm cuonglm force-pushed the cuonglm/make-build-release-work-on-OSX branch from 6e5d84f to 3edd956 Compare November 14, 2019 10:44
@cuonglm cuonglm force-pushed the cuonglm/make-build-release-work-on-OSX branch from 3edd956 to f82ad30 Compare November 14, 2019 10:56
build-release.sh Outdated Show resolved Hide resolved
@cuonglm cuonglm requested a review from na-- November 14, 2019 12:04
@cuonglm cuonglm force-pushed the cuonglm/make-build-release-work-on-OSX branch from 114eba3 to 8d1500b Compare November 14, 2019 15:41
na--
na-- previously approved these changes Nov 14, 2019
@na--
Copy link
Member

na-- commented Nov 14, 2019

Ah, actually, can you add -trimpath in these places as well:
https://github.com/loadimpact/k6/blob/5f12764462a93260eee80d88c749d30552e18749/appveyor.yml#L55
https://github.com/loadimpact/k6/blob/5f12764462a93260eee80d88c749d30552e18749/.circleci/config.yml#L142

And update the PR title and description, since it's fixing more things than the MacOS incompatibilities... 😊

@cuonglm cuonglm force-pushed the cuonglm/make-build-release-work-on-OSX branch from 8d1500b to 83a5337 Compare November 14, 2019 15:57
@cuonglm cuonglm changed the title Make build-release.sh works on Mac OSX Cleanup build script Nov 14, 2019
@cuonglm cuonglm requested a review from na-- November 14, 2019 15:58
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM

It's a shame we can't rely on GNU tools, as find dist -type f | parallel ... would cleanup and speed up the checksum generation (assuming our CI is multi-core). But maybe we can standardize on GNU tools once we move away from Circle and automate these builds.

@cuonglm
Copy link
Contributor Author

cuonglm commented Nov 14, 2019

LGTM

It's a shame we can't rely on GNU tools, as find dist -type f | parallel ... would cleanup and speed up the checksum generation (assuming our CI is multi-core). But maybe we can standardize on GNU tools once we move away from Circle and automate these builds.

You don't need parallel for this task, use xargs instead. But that won't work though, concurrently append to file with >> will make the file content corrupted.

@cuonglm cuonglm merged commit c5f6881 into master Nov 14, 2019
@cuonglm cuonglm deleted the cuonglm/make-build-release-work-on-OSX branch November 14, 2019 16:28
@imiric
Copy link
Contributor

imiric commented Nov 14, 2019

You don't need parallel for this task, use xargs instead.

Ah, but which variant of xargs? ;) Do they all support -P? Also, see man parallel_alternatives. parallel is just nicer to use, though I agree that it's over-engineered (SQL support, remote execution).

But that won't work though, concurrently append to file with >> will make the file content corrupted.

Hhmm true. Would this solve it?

find dist -type f -printf '%P\n' | parallel sha256sum > checksums.txt

In any case, parallelizing this likely won't matter much as a) this script won't be run frequently, and b) there's not a lot of artifacts to checksum, and it's a quick operation anyway.

It would still be good to standardize on a single toolchain, and it should be possible once this is automated.

@cuonglm
Copy link
Contributor Author

cuonglm commented Nov 14, 2019

Ah, but which variant of xargs? ;) Do they all support -P?

both GNU and BSD xargs.

find dist -type f -printf '%P\n' | parallel sha256sum > checksums.txt

It's just the same. The output won't be mixed, but they will be out of order.

It would still be good to standardize on a single toolchain, and it should be possible once this is automated.

I disagree. We shouldn't rely on any toolchain specific features if we can avoid, instead, sticking to POSIX standard, which will be available in all compliant systems. In our case, they're Linux and Mac (BSD). The thing is that for standard utilities, if you write a script which works on BSD, it will be likely working on GNU system as well, but not vice versa.

I invite you to read https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Portable-Shell.html, it's not target POSIX, but it will help you write portable shell script in a better way.

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