Opened 4 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#31253 closed defect (fixed)

Add a webext packaging target to the build script

Reported by: arlolra Owned by: cohosh
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords:
Cc: arlolra, cohosh, phw, dcf Actual Points: .2
Parent ID: Points:
Reviewer: phw Sponsor:

Description

Run npm run pack-webext -- 0.0.x

I imagine that'll do something like,

git clean -x -d -f webext/
require('webext/manifest.json').version = argv[1];
git commit -m "bump version to ${argv[1]}"
git tag argv[1]
git push --tags
npm run webext
zip webext

See comment:6:ticket:31087 for the original suggestion.

Child Tickets

Change History (14)

comment:1 Changed 2 months ago by cohosh

Status: newneeds_review

https://github.com/cohosh/snowflake/pull/10

I opted to not push automatically in the script, but it will make a local commit that updates the manifest to bump the version.

comment:2 Changed 2 months ago by dcf

+  execSync(`git clean -x -d -f .`);

git clean makes me nervous because it has the potential to delete work in progress. Instead of archiving files directly in the working directory, would it be possible to cpoy the files into a temporary directory, and leave the working directory untouched? Something like this may be safer:

npm run webext
TMPDIR=$(mktemp -d webext.XXXXXXXX)
git archive HEAD ./webext | tar -C "$TMPDIR" -xf -
cp webext/snowflake.js "$TMPDIR/webext/"
rm -f webext.zip
(cd "$TMPDIR/webext" && zip -rX ../../webext.zip .)

You need to rm -f webext.zip first, because otherwise zip will modify an existing file, potentially leaving garbage in it. The -X option to zip omits filesystem metadata such as uid/gid. (You can inspect the difference using zipinfo -v webext.zip.)

Here's potentially a better way to make source.zip, that only includes versioned files without the need for git clean:

snowflake/proxy$ git archive -o source.zip HEAD .

What happens if one of the execSync calls fails? Does the whole pack-webext task fail, or does it try to execute the remaining code?

comment:3 in reply to:  2 Changed 2 months ago by cohosh

Replying to dcf:
Thanks, I updated that same link above.

+  execSync(`git clean -x -d -f .`);

I
git clean makes me nervous because it has the potential to delete work in progress. Instead of archiving files directly in the working directory, would it be possible to cpoy the files into a temporary directory, and leave the working directory untouched? Something like this may be safer:

I switched to using the archive option. Note that a call to npm run webext will do a git clean on the webext/ subdirectory anyway.

You need to rm -f webext.zip first, because otherwise zip will modify an existing file, potentially leaving garbage in it. The -X option to zip omits filesystem metadata such as uid/gid. (You can inspect the difference using zipinfo -v webext.zip.)

Done

What happens if one of the execSync calls fails? Does the whole pack-webext task fail, or does it try to execute the remaining code?

The task fails. I added some try-catch code around commands that are likely to fail.

comment:4 Changed 8 weeks ago by arlolra

Something else that belongs in this script is git submodule update --remote so that the translations are up to date before making a release. Not sure that happened for 0.0.11.

From https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=131cf4f8ea0faaa41774e52f5d81878803f45dbc

I opted to not push automatically in the script, but it will make a local commit that updates the manifest to bump the version.

Ok, but note that the tag for 0.0.11 doesn't seem to have been pushed.

https://gitweb.torproject.org/pluggable-transports/snowflake.git/log/

While we're on the subject of versioning, it might be worthwhile to follow semver style major.minor.patch (or, rather, backwards incompat . feature . bugfix)

comment:5 in reply to:  4 Changed 8 weeks ago by cohosh

Replying to arlolra:

Something else that belongs in this script is git submodule update --remote so that the translations are up to date before making a release. Not sure that happened for 0.0.11.

From https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=131cf4f8ea0faaa41774e52f5d81878803f45dbc

Good point, I'll add that to the script. I updated the translations last Friday.

I opted to not push automatically in the script, but it will make a local commit that updates the manifest to bump the version.

Ok, but note that the tag for 0.0.11 doesn't seem to have been pushed.

Should be pushed now.

https://gitweb.torproject.org/pluggable-transports/snowflake.git/log/

While we're on the subject of versioning, it might be worthwhile to follow semver style major.minor.patch (or, rather, backwards incompat . feature . bugfix)

Good point, I'll start updating the minor versions when we have new features.

I was also thinking that it might be useful to add a changelog.txt file that we update whenever we merge changes to the webextension so that it's easier to populate that field in the addon store.

comment:6 Changed 8 weeks ago by arlolra

I was also thinking that it might be useful to add a changelog.txt file that we update whenever we merge changes to the webextension so that it's easier to populate that field in the addon store.

Sounds good

comment:7 Changed 8 weeks ago by cohosh

Just updated snowflake@… with the most recent version: [2019-10-11 16:40:37] Done.

comment:8 Changed 8 weeks ago by cohosh

Noting that I updated the branch above to include a submodule update.

comment:9 Changed 8 weeks ago by cohosh

Actual Points: .2
Owner: set to cohosh
Status: needs_reviewassigned

comment:10 Changed 8 weeks ago by phw

Reviewer: phw
Status: assignedneeds_information

I added two comments (or questions, rather) to the pull request: https://github.com/cohosh/snowflake/pull/10.

comment:11 Changed 8 weeks ago by cohosh

Status: needs_informationneeds_review

Thanks! I commented on the PR and updated one of the try catch blocks to undo changes to the repo and manifest before returning.

comment:12 in reply to:  11 Changed 7 weeks ago by phw

Status: needs_reviewmerge_ready

Replying to cohosh:

Thanks! I commented on the PR and updated one of the try catch blocks to undo changes to the repo and manifest before returning.


Looks good to me.

comment:14 Changed 7 weeks ago by cohosh

Updated the webextension to version 0.0.12

Note: See TracTickets for help on using tickets.