Fix brew formula update #14403

Merged
merged 1 commit into from Jan 6, 2017

Projects

None yet

5 participants

@paulrouget
Contributor
paulrouget commented Nov 29, 2016 edited

Fix servo/saltfs#535

I decided to merge update_brew within upload_nightly.

I hope this will fix the brew formula upadte.


This change is Reviewable

@glennw glennw was assigned by highfive Nov 29, 2016
@highfive

Heads up! This PR modifies the following files:

  • @aneeshusa: etc/ci/upload_nightly.sh, etc/ci/update_brew.sh, etc/ci/buildbot_steps.yml
@paulrouget paulrouget assigned aneeshusa and unassigned glennw Nov 29, 2016
@paulrouget paulrouget referenced this pull request in servo/saltfs Nov 29, 2016
Merged

update_brew doesn't exist anymore #541

@aneeshusa

Sorry about the delay, the approach looks reasonable. This will require a PR to saltfs to pass the homebrew token to this script instead of the update_brew.sh script.

etc/ci/upload_nightly.sh
+ local -r package="${2}"
+ local -r extension="${3}"
+
+ local -r nightly_timestamp="$(date -u +"%Y-%m-%dT%H-%M-%SZ")"
@aneeshusa
aneeshusa Dec 7, 2016 Member

The use of local will always cause this line to exit with a zero return code, masking any errors from the subshell invocation. Hence, it's best to first declare the variable(s) as local, then do the variable assignment (and then optionally mark those variables as readonly afterwards).

@aneeshusa
aneeshusa Dec 7, 2016 Member

Also, I'd move the nightly_timestamp generation/assignment into main, and have main responsible for calling upload and update_brew as necessary.

etc/ci/upload_nightly.sh
+ s3cmd cp "${package_upload_path}" \
+ "${nightly_upload_dir}/servo-latest.${extension}"
+
+ if [[ "${platform}" == "macbrew" ]]
@aneeshusa
aneeshusa Dec 7, 2016 Member

style nit: put the then on the same line

etc/ci/upload_nightly.sh
+ local package_url sha version script_dir tmp_dir
+
+ package_url="https://download.servo.org/nightly/macbrew/${nightly_filename}"
+ sha=$(shasum -a 256 ${package} | sed -e 's/ .*//')
@aneeshusa
aneeshusa Dec 7, 2016 Member

Make sure to double-quote variables and command substitutions per the style guide.

etc/ci/upload_nightly.sh
+ package_url="https://download.servo.org/nightly/macbrew/${nightly_filename}"
+ sha=$(shasum -a 256 ${package} | sed -e 's/ .*//')
+ version=$(echo "${nightly_timestamp}" | \
+ sed -n 's/\([0-9]\{4\}\)-\([0-9]\{2\}\)-\([0-9]\{2\}\).*/\1.\2.\3/p')
@aneeshusa
aneeshusa Dec 7, 2016 Member

Can you add a comment about what this regex is doing?

etc/ci/upload_nightly.sh
+ git commit -m "Version bump: ${version}"
+
+ git push -qf \
+ "https://${TOKEN}@github.com/servo/homebrew-servo.git" master \
@aneeshusa
aneeshusa Dec 7, 2016 Member

Please rename the TOKEN to be less generic, something like HOMEBREW_PUSH_TOKEN.

@bors-servo bors-servo added a commit to servo/saltfs that referenced this pull request Dec 29, 2016
@bors-servo bors-servo Auto merge of #541 - servo:fixBrew, r=aneeshusa
update_brew doesn't exist anymore

follow up servo/servo#14403

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/541)
<!-- Reviewable:end -->
5c911be
@aneeshusa

Just a few last nits.

etc/ci/upload_nightly.sh
+
+ nightly_filename="${2}-$(basename "${1}")"
+ package_url="https://download.servo.org/nightly/macbrew/${nightly_filename}"
+ sha="$(shasum -a 256 ${1} | sed -e 's/ .*//')"
@aneeshusa
aneeshusa Dec 29, 2016 Member

nit: Double-quote "${1}"

@paulrouget
paulrouget Jan 5, 2017 Contributor

Not sure how to properly do that. \" doesn't work for some reason.

@aneeshusa
aneeshusa Jan 5, 2017 Member

$() creates a new quoting context, so you should be able to just do "$(shasum -a 256 "${1}" | sed ...)"

@paulrouget
paulrouget Jan 5, 2017 Contributor

oh right. It works.

etc/ci/upload_nightly.sh
+ sed -n 's/\([0-9]\{4\}\)-\([0-9]\{2\}\)-\([0-9]\{2\}\).*/\1.\2.\3/p')"
+
+ script_dir="${PWD}/$(dirname "${0}")"
+ tmp_dir="$(mktemp -d -t homebrew-servo)"
@aneeshusa
aneeshusa Dec 29, 2016 Member

Add some uppercase Xs to the end of the template, i.e. homebrew-servo.XXXXXXXXX, so not the temporary directory used is randomized.

@paulrouget
paulrouget Jan 5, 2017 edited Contributor

iirc, that's only true on Linux. mktemp on mack adds a postfix automatically.

@aneeshusa
aneeshusa Jan 5, 2017 Member

Oh, I didn't know that, that's a nice feature. Still might be nice to have in case this gets blindly copy-pasted out of context later, but up to you if you want to change it.

etc/ci/upload_nightly.sh
+ "https://${GITHUB_HOMEBREW_TOKEN}@github.com/servo/homebrew-servo.git" \
+ master >/dev/null 2>&1
+
+ rm -rf ${tmp_dir}
@aneeshusa
aneeshusa Dec 29, 2016 Member

Please double-quote "${tmp_dir}".
Also, we're still inside the directory at this point (it's the pwd), so I'm not sure how we're able to delete it...we should cd back to somewhere else (e.g. the "${script_dir}") first.

@aneeshusa
aneeshusa Dec 29, 2016 Member

An alternative to using cd entirely is using the -C argument for the git invocations, e.g. git -C "${tmp_dir}" add ... and editing the path in the sed invocation to "${tmp_dir}/Formula/servo-bin.rb". This avoids cd completely, which I find nicer to reason about.

@paulrouget paulrouget Fix brew formula update
e1654d9
@paulrouget
Contributor

Comments addressed.

@aneeshusa
Member

LGTM, thanks for fixing this! @bors-servo r+

@bors-servo
Contributor

📌 Commit e1654d9 has been approved by aneeshusa

@bors-servo
Contributor

⌛️ Testing commit e1654d9 with merge 7519fc8...

@bors-servo bors-servo added a commit that referenced this pull request Jan 5, 2017
@bors-servo bors-servo Auto merge of #14403 - paulrouget:fixBrew, r=aneeshusa
Fix brew formula update

Fix servo/saltfs#535

I decided to merge update_brew within upload_nightly.

I hope this will fix the brew formula upadte.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14403)
<!-- Reviewable:end -->
7519fc8
@bors-servo
Contributor

💔 Test failed - linux-rel-css

@paulrouget
Contributor
paulrouget commented Jan 6, 2017 edited
@bors-servo
Contributor

⌛️ Testing commit e1654d9 with merge 207f9a5...

@bors-servo bors-servo added a commit that referenced this pull request Jan 6, 2017
@bors-servo bors-servo Auto merge of #14403 - paulrouget:fixBrew, r=aneeshusa
Fix brew formula update

Fix servo/saltfs#535

I decided to merge update_brew within upload_nightly.

I hope this will fix the brew formula upadte.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14403)
<!-- Reviewable:end -->
207f9a5
@bors-servo bors-servo merged commit e1654d9 into servo:master Jan 6, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment