b/66916481: Reenable serializer tests. #214
Conversation
| @@ -0,0 +1,67 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Is it possible to write this as a piece of the gulpfile.js at the top level. Removing the dependency on bash will make it so we can enable windows dev which I am looking in to.
There was a problem hiding this comment.
If it's a ton of work, I have no beef, but it feels like something that is doable.
There was a problem hiding this comment.
Possible? Probably. Worth it? Probably not. Nobody except us should be updating the checked-in protos.
There was a problem hiding this comment.
My issue is more general than this specific use case and more just a "lets make our SDK buildable in multiple envs (i.e. windows, unix)"
There was a problem hiding this comment.
Feel free to take a crack at it, but I don't think it's worth our time. Writing the update.sh at all (rather than just listing the steps in the README.md) was probably a questionable investment.
There was a problem hiding this comment.
(and to be clear, this isn't at all required to build the SDK...)
There was a problem hiding this comment.
Also note that on windows bash is straightforwardly available:
https://msdn.microsoft.com/en-us/commandline/wsl/install_guide
There was a problem hiding this comment.
I guess I haven't used Windows bash, so I tend to stick with JS based tooling. @mikelehen if you could just throw a note in to possibly refactor this to the gulp scripts, I'm fine to go ahead.
There was a problem hiding this comment.
Also it occurs to me that script should be copypastad into the iOS repo (and Android when that happens). The iOS version was manually assembled via copybara before these protos were public but it would be better to just get this from the public repo just like this.
When Android becomes open source it will need this too. Please don't gulpify this.
| pushd "$WORK_DIR" | ||
|
|
||
| # Clone necessary git repos. | ||
| git clone git@github.com:googleapis/googleapis.git |
There was a problem hiding this comment.
This should probably be cloned via https or it will require users without ssh-agent set up to type their passphrases.
There was a problem hiding this comment.
Oh! Interesting. Is that true even for public repos? Anyway, switched it.
| "${PROTOS_DIR}/google/type/" | ||
|
|
||
| mkdir -p "${PROTOS_DIR}/google/protobuf" | ||
| cp protobuf/src/google/protobuf/{any.proto,empty.proto,struct.proto,timestamp.proto,wrappers.proto} \ |
There was a problem hiding this comment.
If you wanted to make this shorter this could factor out .proto too.
protobuf/src/google/protobuf/{any,empty,struct,timestamp,wrappers}.proto
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| set -o nounset |
There was a problem hiding this comment.
Unofficial strict mode for bash is
set -euo pipefail
IFS=$'\n\t'
Rationale here: http://redsymbol.net/articles/unofficial-bash-strict-mode/
It's worth just putting this at the top of every script.
| WORK_DIR=`mktemp -d` | ||
|
|
||
| # check if tmp dir was created | ||
| if [[ ! "$WORK_DIR" || ! -d "$WORK_DIR" ]]; then |
There was a problem hiding this comment.
What are you intending to guard against with ! "$WORK_DIR"? As long as you're quoting $WORK_DIR just checking that it's a directory should be enough here.
Note that if mktemp fails it will return non-zero status and that will fail the script due to set -e.
There was a problem hiding this comment.
I copypasta'd this from elsewhere. I just removed the check entirely given your point about set -e and the fact that this isn't mission critical.
…or iOS / Android eventually.
mikelehen
requested review from
bojeil-google,
gauntface,
sphippen and
wti806
as code owners
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
mikelehen
changed the base branch from
mikelehen-exclude-node-tests-in-browser-2
to
master
|
Closing then reopening to retrigger travis. (I think the base change got things in a weird state) |
This imports the protos we need (and adds an update.sh to re-import in the future) and reenables our serializer tests (for node only).