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

Looks like multiple versions of a crate with a build script clobber each other #100

Closed
mfarrugi opened this issue May 23, 2019 · 10 comments
Closed

Comments

@mfarrugi
Copy link
Collaborator

mfarrugi commented May 23, 2019

cmd = "mkdir -p {{ crate_name_sanitized}}_out_dir_outputs/;"

This ends up making that dir in $execroot/mime_guess_out_dir_outputs, and we have two versions of mime_guess, and it appears they're clobbering each other.

We should have a better idea if that's true and a fix for it in a little bit.

cc @vitalyd

Edit:
I imagine this is partly because sandboxing is disabled for these genrules.

@mfarrugi mfarrugi changed the title Looks like multiple versions of the same crate clobber each other Looks like multiple versions of a crate with a build script clobber each other May 23, 2019
@acmcarther
Copy link
Member

Your explanation makes sense. I think I expected the output to land in a parallel directory structure, not just $execroot. I still have a bad mental model for how Bazel does things.

You can probably use {{ crate.pkg_version }} to make the path unique as a quick and hacky fix. I don't know if that might break for certain filesystem types though it would certainly be possible to sanitize it.

@vitalyd
Copy link
Contributor

vitalyd commented May 23, 2019

@acmcarther, we’re thinking of prefixing the path to mkdir with $$(dirname $@) - that will get the dir for the outs, which should be unique.

There would be a similar change to include that path in the OUT_DIR variable setting.

I’m on mobile and will add a bit more detail tomorrow, but wanted to get your thoughts on this approach.

By the way, why is the genrule template setting local=1? I think removing that yields the parallel directories you were expecting. Presumably there’s a reason for this flag though.

@acmcarther
Copy link
Member

acmcarther commented May 23, 2019

$$(dirname $@) sounds fine to me, but I don't have a deep understanding of the possible pitfalls there. If it functions and doesn't pollute the working dir, it sounds good to me.

I think the local=1 decision is lost to time. I found the commit that includes it, but the description is pathetically bare:
acmcarther/cargo-raze@1258b4d

I did some archeology and I correlated that commit (and "wayland", that it refers to) with this commit in a different project
acmcarther/void@1b43edf

The dependencies there that require build scripts are:

[raze.crates.x11-dl.'2.17.0']
gen_buildrs = true

 [raze.crates.wayland-protocols.'0.12.2']
gen_buildrs = true

 [raze.crates.wayland-client.'0.12.2']
gen_buildrs = true

Trying to compile these with local=0 might be enlightening. If they work without local, then removing it sounds good to me. I can give this a try tomorrow morning I think.

@acmcarther
Copy link
Member

Oh, misunderstanding on my part (though harmless).

Build script generation was originally added in acmcarther/cargo-raze@1a27596#diff-7989f6d09b526681306f518324ef478a. Notably, it does not include local=1. Build script support removed in the following commit, then re-added back in acmcarther/cargo-raze@1258b4d (mentioned in last comment) with local=1.

This could either be because those dependencies required it, or because some the Bazel sandboxing feature was added between the original commit and the re-adding commit.

@mfarrugi
Copy link
Collaborator Author

On the local=1 aspect, apparently that's equivalent to tagging the target as ["no-sandbox", "no-cache", "no-remote"] https://docs.bazel.build/versions/master/be/common-definitions.html

It might make sense to narrow this to no-sandbox, and/or make it non-default.

vitalyd added a commit to vitalyd/cargo-raze that referenced this issue May 24, 2019
@vitalyd
Copy link
Contributor

vitalyd commented May 24, 2019

So I made a PR for adding the $$(dirname $@) to the path in the genrule, but yeah, ideally we can just drop local=1 altogether (or at least not make that the default). The change in the PR is likely harmless even with local=1 removed, although probably best to drop it to not leave unnecessary head-scratchers behind.

@vitalyd
Copy link
Contributor

vitalyd commented May 24, 2019

@acmcarther thanks for the quick merge of the PR. What's the process for getting it published to crates.io? Is that something you (someone else) do manually? Pardon my ignorance 😕

@acmcarther
Copy link
Member

I've been upping the version basically after every PR, but I also want to submit #105 this time. I'll submit that one once CI passes and then publish a new version

@acmcarther
Copy link
Member

Published 0.2.8 which includes #103.

I think #103 fixes the original issue so I'll close this, but please reopen it if you see issues.

@vitalyd
Copy link
Contributor

vitalyd commented May 28, 2019

Great, thanks @acmcarther!

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

No branches or pull requests

3 participants