Skip to content

Conversation

@illicitonion
Copy link
Collaborator

This avoids relying on the system path

The exposed untar attribute is pretty ugly, but I'm not sure the best
alternative. We need to be able to access untar from anything which
has an out_dir_tar attribute, which may be any of our rules, but need
to specifically not depend on it from the targets which make untar
itself. This means it can't live in a toolchain.

Ideally it'd be an attribute conditionally set only iff out_dir_tar is
set, which we could do in a macro, but introducing a layer of macros to
the rules just to make our handful of bootstrap rust_libraries not need
to fake out the attribute seems overkill. The binary will only
actually be depended on and built if it's needed.

Everything except the top commit is #346

@damienmg
Copy link
Collaborator

#346 being merged, do you mind rebasing? Thanks!

This avoids relying on the system path

The exposed `untar` attribute is pretty ugly, but I'm not sure the best
alternative. We need to be able to access `untar` from anything which
has an `out_dir_tar` attribute, which may be any of our rules, but need
to specifically not depend on it from the targets which make `untar`
itself. This means it can't live in a toolchain.

Ideally it'd be an attribute conditionally set only iff `out_dir_tar` is
set, which we could do in a macro, but introducing a layer of macros to
the rules just to make our handful of bootstrap rust_libraries not need
to fake out the attribute seems overkill. The binary will only
_actually_ be depended on and built if it's needed.
@illicitonion
Copy link
Collaborator Author

Rebased - thanks!

CI is currently failing in the examples dir because this puts a requirement on having a few repositories bound into the workspace - I guess this is something we can put into the rust_repositories function, but curious if there are better ideas

#[test]
fn fails() {
let mut dir = temp_dir();
let now_millis = SystemTime::now().duration_since(UNIX_EPOCH).expect("You're not before 1970, surely?").as_millis();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let now_millis = SystemTime::now().duration_since(UNIX_EPOCH).expect("You're not before 1970, surely?").as_millis();
let now_millis = SystemTime::now().duration_since(UNIX_EPOCH).expect("Did you time travelled?").as_millis();

;)

@damienmg
Copy link
Collaborator

CI is currently failing in the examples dir because this puts a requirement on having a few repositories bound into the workspace - I guess this is something we can put into the rust_repositories function, but curious if there are better ideas

I think the best approach is rust_repositories, however this is not really nice to have that much deps for such a feature.

I would lean toward deprecating the support for tarball, do we actually have a use-case that justify keeping this feature around?

@illicitonion
Copy link
Collaborator Author

Yeah, I think I agree that deprecating the out_dir_tar attribute is a preferable solution. I'm not sure of any important use-cases - it does feel like build_script support is probably a sufficient replacement.

@damienmg
Copy link
Collaborator

@dfreese @smklein @acmcarther @mfarrugi Any reason to keep out_dir_tar around now that we have a build_script rule?

@smklein
Copy link
Contributor

smklein commented Jun 22, 2020

@dfreese @smklein @acmcarther @mfarrugi Any reason to keep out_dir_tar around now that we have a build_script rule?

No reason that I'm aware of - I only saw it used for the build scripts. "out_dir_tar" is no longer referenced by cargo-raze anymore either.

@damienmg
Copy link
Collaborator

damienmg commented Jun 23, 2020

@illicitonion Do you mind doing a change to simply add a failure if someone use out_dir_tar instead pointing them at the build script rule?

@damienmg
Copy link
Collaborator

This is now superceded by #354 which is needed for windows support.

@damienmg damienmg closed this Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants