search

temporalio / sdk-core

Core Temporal SDK that can be used as a base for language specific Temporal SDKs
MIT License
266 stars 71 forks source link

Move protos to sdk-core-protos #638

Closed cole-h closed 10 months ago

cole-h commented 10 months ago

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!

CLAassistant commented 10 months ago

CLA assistant check
All committers have signed the CLA.

Sushisource commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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

cole-h commented 10 months ago

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 commented 10 months ago

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