[mxgrey]

This PR attempts to fix #233 by comparing uncompressed representations of the frontend (.tar instead of .tar.gz) when checking for potential changes. Since the tar building mode is deterministic, the uncompressed versions of the frontend should be identical.

@sauk2 I didn't manage to reproduce your problem locally, so I would appreciate your help in testing this out. If it fixes the problem that you're experiencing then we can merge and release this.

[koonpeng]

I am able to repro the bug and confirm that this fix works. But I made a table for something to think about in terms of future maintainability.

commit dist: The current approach, commits dist.tar.gz as the SOT.
cargo test in CI ensures the tarball is up to date. But committing a binary tarball is a red flag.

always build dist: Equivalent to always building with BUILD_FRONTEND=1, gitignore dist.tar.gz.
build.rs is not included in the crate so cargo package (and cargo publish) will fail if frontend is not built prior. Nothing detects if an outdated frontend is being packaged into the crate. Responsibility lies in the maintainer.

include frontend source in crate: Basically have downstream build the frontend.
Requires downstream to have js stack.

	commit dist	always build dist	include frontend source in crate
Downstream requires js stack?	No	No	Yes
Server contributors need js stack?	Yes (No if we drop verification in build.rs, CI still ensures it is always up to date)	Yes	Yes
Safe cargo publish?	Yes (unless ran with --allow-dirty)	No	Yes
Other red flags	Committing binary tarball in git. Byte level hash of the tarball depends on committer system. Downstream builds differently from source (no build.rs in crate)	Downstream builds differently from source (no build.rs in crate)	N/A

[mxgrey]

But committing a binary tarball is a red flag.

I've been wondering about this exact concern. We haven't seen any significant issues with it yet, but I want to make sure we're exercising good git etiquette.

build.rs is not included in the crate

Could you elaborate on what you mean by this? The build.rs file is always shipped as part of the published crate and gets run when building a downstream crate.

Thinking over the three options, I think what we're already doing provides the best user experience. I do worry about committing the tar ball to our git history, though. What if instead of committing the whole tar ball, we only commit a SHA-256 hash of the tar ball and compare hashes when running verify_frontend? The risk of a hash collision should be virtually zero.

[koonpeng]

Could you elaborate on what you mean by this? The build.rs file is always shipped as part of the published crate and gets run when building a downstream crate.

https://github.com/open-rmf/bevy_impulse/blob/main/diagram-editor/Cargo.toml#L23. Since we use include, cargo will exclude all non-include files so downstream doesn't have build.rs.

What if instead of committing the whole tar ball, we only commit a SHA-256 hash of the tar ball and compare hashes when running verify_frontend?

We can't hash the whole tarball because of unreliable byte level compression, but we can hash every file in dist. In practice, it's not very different from always building dist.tar.gz. The advantage of committing the dist is that it blocks cargo publish if the git status is dirty. Without commit dist, it runs the risk of checking out another branch, build, checkout main, and attempt to publish, resulting in publishing the wrong frontend.

Since this really only impacts the maintainer, it can be an acceptable risk. The maintainer just need to be aware to always build on a clean checkout before publishing. Normally cargo publish will do a local build to verify before publishing, but since we deliberately omit build.rs, cargo's verification doesn't work.

Edit: on 2nd thought, committing hash can work if the verification mode compare the hash to the gitignored dist.tar.gz. Then we can include build.rs in the crate.

[mxgrey]

Edit: on 2nd thought, committing hash can work if the verification mode compare the hash to the gitignored dist.tar.gz. Then we can include build.rs in the crate.

Yep, this is what I had in mind. I'll experiment with this and see if it works as expected.

[Briancbn]

Thanks for quickly providing a fix. Works like charm!

Hash instead of tar.gz in commits would be better.