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

Move protos to sdk-core-protos #638

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Move protos to sdk-core-protos #638

merged 1 commit into from
Nov 20, 2023

Conversation

cole-h
Copy link
Contributor

@cole-h cole-h commented Nov 20, 2023

What was changed

I moved the protos folder from the root of the project into the sdk-core-protos directory, and added a symlink to this new location at the root of the repository.

Why?

Without this change, Nix builds of Rust projects that depend on this will fail with the following error:

Error: Custom { kind: Other, error: "protoc failed: Could not make proto path relative: ../protos/local/temporal/sdk/core/core_interface.proto: No such file or directory\n" }

This is because the way Nix builds dependency crates is to separate each crate into its source (so sdk-core-protos would be its own directory, and would be in an environment totally unlike the current repository's structure). Its parent directory .. would not contain a protos directory to rely on.

With this change, the protos directory is inside sdk-core-protos, and accessing child files and directories is allowed, so accessing ./protos works and the build succeeds!

Checklist

  1. How was this tested:

    • If you have Nix installed, you can run nix build github:cole-h/sdk-core-test/failure to see the errored build before this change, and nix build github:cole-h/sdk-core-test/success to see the successful build after this change. (And you can visit https://github.com/cole-h/sdk-core-test to see the source code involved in reproducing.)
    • I also verified that everything still compiles with cargo check and all the tests pass with cargo test (I noticed some tests include_str some protocols directly -- those still work for me).
  2. Any docs updates needed?

    • I updated the REAME docs, and I didn't see anything on docs.temporal.io that would need to be updated.

I don't think this is a significant code change that would require opening an issue, however, I would gladly be told otherwise!

@cole-h cole-h requested a review from a team as a code owner November 20, 2023 16:21
@CLAassistant
Copy link

CLAassistant commented Nov 20, 2023

CLA assistant check
All committers have signed the CLA.

@Sushisource
Copy link
Member

I'm fine with this, but Core's dependencies often reach into this directory as well in order to generate their own protos. The symlink should largely deal with that, but, possibly not on Windows where git doesn't always create them properly.

I'll run this by the other people on my team before merging it just to make sure it's not going to be disruptive to dependencies

@cole-h
Copy link
Contributor Author

cole-h commented Nov 20, 2023

That's true, I forgot that Windows doesn't always play well with git+symlinks... If you have any ideas on how to improve this case, I'd be willing to give it a shot!

But also, no worries if this is something you don't want to deal with right now -- we'll only need to rebase this branch whenever we update any of these dependencies, which isn't that big of a deal. (I feel like that could be taken sarcastically, so please know that I am being 100% genuine: it's no big deal!)

@Sushisource
Copy link
Member

@cole-h We're fine with this, but let's actually not even use the symlink at all. It's easy enough for us to just handle the new directory when we update the core dependency and it avoids having things seem like they work on one platform and not another.

Will merge after that

Nix sandboxes builds such that they cannot escape their manifest
directory. By moving the `protos` into `sdk-core-protos`, Nix will be
able to build crates that depend on `sdk-core-protos`.

Otherwise, the build script will fail:

    Error: Custom { kind: Other, error: "protoc failed: Could not make proto path relative: ../protos/local/temporal/sdk/core/core_interface.proto: No such file or directory\n" }
@cole-h
Copy link
Contributor Author

cole-h commented Nov 20, 2023

OK, I've removed the symlink and updated the tests to point at the new location under sdk-core-protos! Thanks a lot!

@cole-h cole-h changed the title Move protos to sdk-core-protos, add symlink in root Move protos to sdk-core-protos Nov 20, 2023
@Sushisource Sushisource merged commit 2878bab into temporalio:master Nov 20, 2023
5 checks passed
@cole-h cole-h deleted the protos-fix-build-with-nix branch November 21, 2023 00:03
@cole-h
Copy link
Contributor Author

cole-h commented Nov 21, 2023

Thanks for being open to this! You guys are awesome!

@dandavison dandavison mentioned this pull request Nov 25, 2023
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.

3 participants