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

chore: Run codegen with N=5, fix update-bins #418

Merged
merged 4 commits into from
Nov 27, 2021
Merged

chore: Run codegen with N=5, fix update-bins #418

merged 4 commits into from
Nov 27, 2021

Conversation

stephenh
Copy link
Owner

@stephenh stephenh commented Nov 27, 2021

We had some old .bin files (that were more than maxdepth 2), and we actually generate imported files potentially twice (i.e. timestamp.ts is in both foo.bin and bar.bin) and so the order of which bin-d version of timestamp we saw would be dependent on the bash-level parallelization picking which file got ran first:

$ yarn bin2ts type-registry
yarn run v1.22.5
$ docker-compose run --rm protoc codegen.sh type-registry
Creating ts-proto_protoc_run ... done
Generating typescript code for integration tests using 5 cores...
type-registry/foo.bin
type-registry/bar/bar.bin
READING type-registry/foo.bin
GOT google/protobuf/timestamp.proto { path: [], span: [ 30, 0, 146, 1 ], leadingDetachedComments: [] }
READING type-registry/bar/bar.bin
GOT google/protobuf/timestamp.proto { path: [], span: [ 30, 0, 137, 1 ], leadingDetachedComments: [] }
GOT foo.proto { path: [], span: [ 0, 0, 12, 1 ], leadingDetachedComments: [] }
done
GOT foo.proto { path: [], span: [ 0, 0, 12, 1 ], leadingDetachedComments: [] }
GOT bar/bar.proto { path: [], span: [ 0, 0, 8, 1 ], leadingDetachedComments: [] }
done
Done in 3.85s.

@stephenh stephenh changed the title chore: Run codegen with five procs chore: Run codegen with N=5, fix update-bins Nov 27, 2021
@stephenh stephenh merged commit eb46583 into main Nov 27, 2021
@stephenh stephenh deleted the run-with-five branch November 27, 2021 17:55
@stephenh
Copy link
Owner Author

🎉 This PR is included in version 1.91.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@boukeversteegh
Copy link
Collaborator

Hi @stephenh! Thank you for fixing the issues with the binary files!

I see you have simplified the Dockerfile and docker-compose file a bit, which is fine I guess, but now it's no longer possible to use the protoc alias as a transparent dockerized version of protoc.

Do you mind if I brought back the $PWD auto mounting into the docker container, and updated the aliases? Or do you consider including a dockerized protoc version for general use out of scope for this project? In the latter case, I will remove the relevant section from the readme that describes its use. Cheers!

@stephenh
Copy link
Owner Author

Hey @boukeversteegh ! Yeah, those changes were part of my previously mentioned "...let's just try things and hope they work...", but yeah then I was lazy about putting them back (and, of course/as usual, the problem was not in "wtf is docker/docker-compose not saving our changes incorrectly?!?!!" but our own silly bug :-)).

Ah okay, okay, I looked at what you'd setup some more, and I think what I missed originally was that, for me personally, the protoc / aliases.sh / etc. wouldn't be part of day-to-day / working-on-ts-proto itself workflow (afaiu, i.e. I just use the codegen/yarn commands all the time), so I was kind "eh" at first.

But I think I get now that it'd actually be pretty useful for users of ts-proto to use it on their own projects i.e. a great way of using protoc and ts-proto without the gymnastics of installing a native protoc.

I kind of wonder, instead of having to git clone ts-proto and run docker-compose build in our working copy, then run aliases.sh and go over to their own project, wydt about creating a ts-proto docker image, and publishing it to docker hub?

Like something like, during CI:

  • First publish the latest version of ts-proto to npm
  • Then run docker build which installs protoc and the just-published ts-proto from npm
  • Then publishes to docker hub as the same version tag as the ts-proto in npm

I guess users would still need their own docker-compose so that they could mount their project's working directory...

Seems like that would be pretty cool? If it's too ambitious though, and your setup was accomplishing the same thing but w/o the docker hub publishing step, then yep I'm good with bringing it back. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants