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

Make proto/ vendor-able #128

Merged
merged 3 commits into from Sep 9, 2021
Merged

Make proto/ vendor-able #128

merged 3 commits into from Sep 9, 2021

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Sep 9, 2021

I work with a large monorepo that cargo vendor's all third-party code. We use tokio quite a bit, and want to experiment with tokio/console

When vendoring console-api the ../proto directory is lost, and the crate is unbuildable. I understand that keeping proto/ as a top-level directory is probably desirable, so this change symlinks that directory into that sub-crate itself.

My understanding is that this change may also be required once console is put on crates.io?

cargo vendor before this change:

Cargo.toml build.rs   src

after:

Cargo.toml
build.rs
proto
src

This is basically copying what https://github.com/dtolnay/cxx does with symlinks

@hawkw
Copy link
Member

hawkw commented Sep 9, 2021

Thanks, it hadn't occurred to me that this would be a potential issue for users who are vendoring.

Re:

I understand that keeping proto/ as a top-level directory is probably desirable, so this change symlinks that directory into that sub-crate itself.

FWIW, I think it would probably be fine if we did move proto/ into console-api, since I would generally expect most (or all?) Rust code to consume the protobufs via that crate rather than directly, and I am not sure if it's likely that they'll be used outside of Rust any time in the near future. But, the symlink is fine, too, just wanted to put it out there that I don't think not moving the proto definitions is a hard blocker.

@guswynn
Copy link
Contributor Author

guswynn commented Sep 9, 2021

@hawkw did some random commits accidentally get added to this PR?

@hawkw
Copy link
Member

hawkw commented Sep 9, 2021

@hawkw did some random commits accidentally get added to this PR?

We have GitHub configured to require that branches are up to date before merging them (to ensure CI runs with the branch integrated with the latest changes). Therefore, I clicked the "Update this branch with the latest changes" button to prepare to merge it.

@hawkw hawkw merged commit 81cd611 into tokio-rs:main Sep 9, 2021
@spacemeowx2
Copy link

Hi, I found that this PR makes the console not compile in Windows and cross. I think it's because of the symbolic links.

Error message is like:

Caused by:
  process didn't exit successfully: `/target/release/build/console-api-a01a908c13fc7610/build-script-build` (exit status: 1)
  --- stderr
  Error: Custom { kind: Other, error: "protoc failed: Could not make proto path relative: proto/trace.proto: No such file or directory\n" }

hawkw added a commit that referenced this pull request Sep 13, 2021
Currently, the protobuf definitions for the console API are symlinked
into the `console-api` directory. This is intended to support vendoring
the `console-api` crate (see #128).

However, some users have reported that the symlink isn't the ideal
solution. Windows users have had issues following the Unix symlink, so
#128 broke the build for people trying to develop the console on Windows
(I suspect it may still work in WSL?).

There's no _real_ reason that the `proto/` directory needs to be in the
repo root, rather than the `console-api` crate; the symlink was only
added to avoid moving it around. Therefore, it's fine to just move it
instead. This branch moves the `proto/` dir into `console-api/proto`,
hopefully fixing the build issues for Windows users.
@hawkw
Copy link
Member

hawkw commented Sep 13, 2021

Hi, I found that this PR makes the console not compile in Windows and cross. I think it's because of the symbolic links.

Error message is like:

Caused by:
  process didn't exit successfully: `/target/release/build/console-api-a01a908c13fc7610/build-script-build` (exit status: 1)
  --- stderr
  Error: Custom { kind: Other, error: "protoc failed: Could not make proto path relative: proto/trace.proto: No such file or directory\n" }

Yeah, other users have reported that as well. PR #141 should fix this...but unfortunately, I don't have easy access to a Windows machine to test that on. If you're interested in checking out that branch and confirming that it does fix the build, that would be lovely!

@guswynn
Copy link
Contributor Author

guswynn commented Sep 13, 2021

@spacemeowx2 I don't have access to a windows machine, can you try building doing this? https://github.com/dtolnay/cxx/blob/master/.github/workflows/ci.yml#L36

in the meantime, I think @hawkw is right, #141 will simplify this

Sorry about the noise!

hawkw added a commit that referenced this pull request Sep 13, 2021
Currently, the protobuf definitions for the console API are symlinked
into the `console-api` directory. This is intended to support vendoring
the `console-api` crate (see #128).

However, some users have reported that the symlink isn't the ideal
solution. Windows users have had issues following the Unix symlink, so
#128 broke the build for people trying to develop the console on Windows
(I suspect it may still work in WSL?).

There's no _real_ reason that the `proto/` directory needs to be in the
repo root, rather than the `console-api` crate; the symlink was only
added to avoid moving it around. Therefore, it's fine to just move it
instead. This branch moves the `proto/` dir into `console-api/proto`,
hopefully fixing the build issues for Windows users.
@spacemeowx2
Copy link

@guswynn Unfortunately it doesn't work for me:

error: unable to create symlink console-api/proto: Permission denied

@hawkw This PR fixed my problem.

Thanks!

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.

None yet

3 participants