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 Travis to make releases to Github #83

Merged
merged 1 commit into from
Jun 22, 2016
Merged

Use Travis to make releases to Github #83

merged 1 commit into from
Jun 22, 2016

Conversation

andreastt
Copy link
Contributor

@andreastt andreastt commented Jun 13, 2016

This change is Reviewable

@andreastt
Copy link
Contributor Author

@jgraham Ready for review.

@andreastt
Copy link
Contributor Author

Updated with token.

@jgraham
Copy link
Member

jgraham commented Jun 15, 2016

Reviewed 1 of 1 files at r3.
Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.


.travis.yml, line 46 [r2] (raw file):

deploy:
  provider: releases
  api_key: "TOKEN"

I assume this shouldn't be the literal string "token"


.travis.yml, line 49 [r2] (raw file):

  skip_cleanup: true
  file:
    - "geckodriver-$TRAVIS_TAG.tar.gz"

I think it might be good to make these filenames match the existing scheme better?


ci.sh, line 99 [r3] (raw file):

      file geckodriver-$1-$2.zip
  else
      tar zcvf geckodriver-$1-$2.tar.gz $bin

Why .tar? It's a single file so tar does nothing useful.


Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Jun 15, 2016

Sorry I didn't press publish earlier. I think this is kind of OK, but I'm not thrilled about a hundred lines of shell script in the build system :/


Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@andreastt
Copy link
Contributor Author

Shell scripts are a perfectly normal way to deal with software packaging because what you really want is some conditional behaviour, some input/output, and ability to easily run shell commands.

But as we discussed earlier, I can foresee most of this going away once we get cargo vendoring support in central or something similar to that. Although I agree it could be nice to have the packaging steps as separate commands you could run locally. I might address this in a future patch.


Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.


ci.sh, line 99 [r3] (raw file):

Previously, jgraham wrote…

Why .tar? It's a single file so tar does nothing useful.

I think what we want to do here is put it inside a directory with the version number, like we do in the `package_source` function below. I will address this.

Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Jun 15, 2016

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.


ci.sh, line 99 [r3] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

I think what we want to do here is put it inside a directory with the version number, like we do in the package_source function below. I will address this.

I think it would be better to just zip up the file. Why would we benefit from untaring producing a file with the version number?

Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Jun 15, 2016

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.


ci.sh, line 99 [r3] (raw file):

Previously, jgraham wrote…

I think it would be better to just zip up the file. Why would we benefit from untaring producing a file with the version number?

*gzip

Comments from Reviewable

@andreastt
Copy link
Contributor Author

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.


ci.sh, line 99 [r3] (raw file):

Previously, jgraham wrote…

*gzip

Because it is the standard practice when producing tarballs of programs.

Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Jun 16, 2016

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


ci.sh, line 99 [r3] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Because it is the standard practice when producing tarballs of programs.

I'm still opposed to this. I don't know what value it provides to anyone, but it means that you extract extra files that aren't needed. For example chromedriver doesn't do this.

Comments from Reviewable

@andreastt
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


.travis.yml, line 49 [r2] (raw file):

Previously, jgraham wrote…

I think it might be good to make these filenames match the existing scheme better?

Done.

ci.sh, line 99 [r3] (raw file):

Previously, jgraham wrote…

I'm still opposed to this. I don't know what value it provides to anyone, but it means that you extract extra files that aren't needed. For example chromedriver doesn't do this.

I’ve changed the packages to include the license and readme file, which we have to ship with the program anyway.

Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Jun 20, 2016

Reviewed 1 of 2 files at r5.
Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.


.travis.yml, line 49 [r2] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Done.

I don't think this changed? I would really like names that aren't this confusing for people trying to figure out the right one to use.

ci.sh, line 99 [r3] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

I’ve changed the packages to include the license and readme file, which we have to ship with the program anyway.

OK, I guess. But I think that gzipping up just the binary and making `--help`more useful would be a better option. I don't think there's any requirement to ship a LICENSE file. Indeed afaict the only requirement in the license that's relevant here is to tell people where they can get the source, which neither the LICENSE file nor the README.md file actually do.

Comments from Reviewable

@andreastt
Copy link
Contributor Author

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.


.travis.yml, line 49 [r2] (raw file):

Previously, jgraham wrote…

I don't think this changed? I would really like names that aren't this confusing for people trying to figure out the right one to use.

Okay, I forgot to change which files Travis chooses to upload, but the file names produced _on_ Travis has changed.

Comments from Reviewable

@andreastt
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


.travis.yml, line 49 [r2] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Okay, I forgot to change which files Travis chooses to upload, but the file names produced on Travis has changed.

Fixed the filenames in 61e2195.

ci.sh, line 99 [r3] (raw file):

Previously, jgraham wrote…

OK, I guess. But I think that gzipping up just the binary and making --helpmore useful would be a better option. I don't think there's any requirement to ship a LICENSE file. Indeed afaict the only requirement in the license that's relevant here is to tell people where they can get the source, which neither the LICENSE file nor the README.md file actually do.

Okay, I’m changing it back to the way it was. I’ve filed https://github.com//issues/107 to improve the `-h` text.

Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Jun 22, 2016

Reviewed 1 of 1 files at r9, 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

When a tag is pushed we will following this patch also do a release
build if the debug build and tests passes. We will then being packaging
for each target, along with an archive for the source code. The packages
are named after the targets.

We do a normal debug build and tests first because these are cheaper to
do and will give a shorter turnaround time for developers if any tests
fail. You can also only run tests with debug builds.

Fixes #72.
@andreastt andreastt changed the title Use travis to make releases to Github Use Travis to make releases to Github Jun 22, 2016
@jgraham
Copy link
Member

jgraham commented Jun 22, 2016

Reviewed 2 of 2 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jgraham jgraham merged commit 54a9643 into mozilla:master Jun 22, 2016
@jgraham
Copy link
Member

jgraham commented Jun 22, 2016

Thanks! Sorry this took so many twists and turns.

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.

2 participants