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

Add an example using Git for dependency resolution #5

Merged
merged 3 commits into from
Mar 16, 2018
Merged

Add an example using Git for dependency resolution #5

merged 3 commits into from
Mar 16, 2018

Conversation

bspeice
Copy link
Contributor

@bspeice bspeice commented Mar 15, 2018

Should help in testing google/cargo-raze#22 and google/cargo-raze#28.
Currently broken, as BUILD files aren't generated in Git vendored dependencies.

@bspeice
Copy link
Contributor Author

bspeice commented Mar 15, 2018

Note that code in google/cargo-raze#22 is broken in that it finishes without generating BUILD files in the git-vendored libraries, google/cargo-raze#28 is currently broken with some panicking .unwrap() calls.

@acmcarther
Copy link
Owner

Thanks for the additional example!

The repo structure is a bit weird, and as-is this example is going to get clobbered next time ./internal/update-master.sh is run. What that script does is copy the example directories from ./internal/sources into ./bazel, then run cargo-raze for them.

Can you move the example (sans vendored crates +generated BUILDs) into ./internal/sources as well so that update-master.sh can see it?

@acmcarther
Copy link
Owner

Ah, also can you update the new sanity_check.sh script to match #4 (putting both of the builds on the same line joined with &&)?

@bspeice
Copy link
Contributor Author

bspeice commented Mar 16, 2018

Waiting for Travis, but I think this is what you were going for?
Can I also make a recommendation of marking bazel/ in the .gitignore so that it's more obvious where things should be added for other people contributing things in the future?

@bspeice
Copy link
Contributor Author

bspeice commented Mar 16, 2018

The error from Travis, of which I have no idea what's going on:

error: use of unstable library feature 'rustc_private': this crate is being loaded from the sysroot, and unstable location; did you mean to load this crate from crates.io via `Cargo.toml` instead? (see issue #27812)
  --> non_cratesio_library/src/main.rs:22:3
   |
22 |   info!("Got result from future: {:?}", result);
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a macro outside of the current crate

@acmcarther
Copy link
Owner

You're missing an explicit dependency on log in the example.

https://github.com/bspeice/cargo-raze-examples/blob/master/internal/sources/non_cratesio_library/BUILD

For some reason you can sometimes depend on crates without explicitly depending on them, but only in unstable, and only for certain crates.

See rust-lang/rust#27812 for details

@bspeice
Copy link
Contributor Author

bspeice commented Mar 16, 2018

Which is strange, because it's definitely included in the Cargo.toml.
EDIT: I'm an idiot, I was looking at the wrong BUILD file to notice that it wasn't being included properly.

@acmcarther
Copy link
Owner

I linked the handwritten example BUILD file for the example (not a generated build file). You missed the //non_cratesio_library/cargo:log dep.

@acmcarther
Copy link
Owner

This LGTM. I feel like we're going to want to probably reduce the number of whole examples back to two-ish (one per genmode) to reduce the number of redundant compilations of crates and speed up CI, but its not a big deal for now. Not yet sure the best way to do that and still have the features "accessible".

Thanks again!

@acmcarther acmcarther merged commit c39c85a into acmcarther:master Mar 16, 2018
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.

2 participants