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

Search for Xargo.toml in parent directories, including test #254

Merged
merged 2 commits into from
Sep 19, 2019

Conversation

Nils-TUD
Copy link
Contributor

@Nils-TUD Nils-TUD commented Sep 9, 2019

So far, xargo expected the Xargo.toml in the directory of the Cargo.toml. This patch searches for the Xargo.toml by walking up the directory hierarchy. This is useful if a workspace is used, leading, for example, to one Cargo.toml with the workspace and multiple Cargo.toml files for the members of the workspace. With this patch, it is no longer necessary to have one Xargo.toml for each Cargo.toml, but only one next to the workspace, for example.

@Nils-TUD
Copy link
Contributor Author

Nils-TUD commented Sep 9, 2019

The test "test" seems to be a bit unstable. It failed for me a couple of times as well, but now it works.

@RalfJung
Copy link
Collaborator

RalfJung commented Sep 9, 2019

That's a network failure indeed. I restarted that build.

@RalfJung
Copy link
Collaborator

RalfJung commented Sep 9, 2019

And now this fails due to rust-lang/rust#64320. We'll have to wait until that lands.

@RalfJung
Copy link
Collaborator

All right, this should work now then.

bors r+

bors bot added a commit that referenced this pull request Sep 11, 2019
254:  Search for Xargo.toml in parent directories, including test r=RalfJung a=Nils-TUD

So far, xargo expected the Xargo.toml in the directory of the Cargo.toml. This patch searches for the Xargo.toml by walking up the directory hierarchy. This is useful if a workspace is used, leading, for example, to one Cargo.toml with the workspace and multiple Cargo.toml files for the members of the workspace. With this patch, it is no longer necessary to have one Xargo.toml for each Cargo.toml, but only one next to the workspace, for example.

Co-authored-by: Nils Asmussen <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 11, 2019

Build failed

@RalfJung
Copy link
Collaborator

Looks like your test does not work with the 2018-12-01 nightly?

running: "cc" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "-g" "-fno-omit-frame-pointer" "-mthumb" "-march=armv7-m" "-fno-builtin" "-fvisibility=hidden" "-ffreestanding" "-DVISIBILITY_HIDDEN" "-o" "/tmp/xargo.gIvG1WHQOidU/target/thumbv7m-parent-eabi/debug/build/compiler_builtins-588acbe8be9479e8/out/../../libcompiler_builtins/compiler-rt/lib/builtins/absvdi2.o" "-c" "../../libcompiler_builtins/compiler-rt/lib/builtins/absvdi2.c"

cargo:warning=cc: error: unrecognized command line option ‘-mthumb’

Is this a fairly recent target you are testing here? Could you use one used by other tests (and thus likely supported for longer already)?

@Nils-TUD
Copy link
Contributor Author

Nils-TUD commented Sep 11, 2019

That's weird, because I did already copy the target from another test (and changed the middle part, as the other tests do, because apparently it's a problem if two have the same name). The target_dependencies test uses thumbv7m-none-eabi, which works according to the log. I'm not sure what's going on there...

Edit: And it's calling gcc, so that's too old apparently. But why isn't that called for the other test?

@RalfJung
Copy link
Collaborator

I am not sure either. I don't know anything about all this ARM/cross-compilation stuff.^^

Maybe it has something to do with the "none" target also occurring here, whereas your target does not?

@Nils-TUD
Copy link
Contributor Author

Ok, it is interesting that the two other tests that have the comment "need this exact target name to get the right gcc flags" use an existing target (with "none"). So, maybe this is the problem.

As far as I can see, it should be an existing target, but cannot be a target that the host has already built. Otherwise it will find the alloc library in any case.

Let's try riscv32i-unknown-none-elf :)

@RalfJung
Copy link
Collaborator

That does not seem to work...

As far as I can see, it should be an existing target, but cannot be a target that the host has already built. Otherwise it will find the alloc library in any case.

Parallelism is another problem here -- tests are run in parallel. That's why the "host" tests have locking logic.

@RalfJung
Copy link
Collaborator

What about modifying an existing test to have the toml file in a parent directory?

@Nils-TUD
Copy link
Contributor Author

That does not seem to work...

But interestingly, my test (target_dependencies_parentdir) works. Only host_patch
and test fail. And they fail, because backtrace is not found. I don't see why that is related.

What about modifying an existing test to have the toml file in a parent directory?

That's exactly what I did, based on the target_dependencies test. But I can't use the same target, because this apparently causes those two tests to interfere with each other.

I'm a bit lost here. It works for me locally and with travis, only other and unrelated tests fail. Any suggestions?

@RalfJung
Copy link
Collaborator

RalfJung commented Sep 18, 2019

But interestingly, my test (target_dependencies_parentdir) works. Only host_patch
and test fail. And they fail, because backtrace is not found. I don't see why that is related.

Ah, I should have checked the log then. ;)
This was an instance of rust-lang/rust#64410.

That's exactly what I did, based on the target_dependencies test. But I can't use the same target, because this apparently causes those two tests to interfere with each other.

No, what I meant is not to copy and modify, but to modify in-place.
But if what you got now works, that's even better.

@RalfJung
Copy link
Collaborator

Thanks a lot!

bors r+

bors bot added a commit that referenced this pull request Sep 18, 2019
254:  Search for Xargo.toml in parent directories, including test r=RalfJung a=Nils-TUD

So far, xargo expected the Xargo.toml in the directory of the Cargo.toml. This patch searches for the Xargo.toml by walking up the directory hierarchy. This is useful if a workspace is used, leading, for example, to one Cargo.toml with the workspace and multiple Cargo.toml files for the members of the workspace. With this patch, it is no longer necessary to have one Xargo.toml for each Cargo.toml, but only one next to the workspace, for example.

Co-authored-by: Nils Asmussen <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 18, 2019

Build failed

@Nils-TUD
Copy link
Contributor Author

Why does it try to build the arm/aeabi_cdcmp.S when building for riscv?

hm, maybe I should really just modify the target_depencencies test in-place?

@RalfJung
Copy link
Collaborator

I'm afraid I don't know.

We could change the minimal supported version to nighty-2019-01-01 (mostly because that's a much less arbitrary date :D ), but I am not sure if making it one month younger will help with whatever issue you are running into here.

@Nils-TUD
Copy link
Contributor Author

I'm also fine with that, but I would suggest that I simply modify an existing test. I don't think that we lose anything with that.

This commit does not add a new test, because a unique and existing
target is required, which turned out to be challenging.
@RalfJung
Copy link
Collaborator

Yeah, this is fine. Thanks!

bors r+

bors bot added a commit that referenced this pull request Sep 19, 2019
254:  Search for Xargo.toml in parent directories, including test r=RalfJung a=Nils-TUD

So far, xargo expected the Xargo.toml in the directory of the Cargo.toml. This patch searches for the Xargo.toml by walking up the directory hierarchy. This is useful if a workspace is used, leading, for example, to one Cargo.toml with the workspace and multiple Cargo.toml files for the members of the workspace. With this patch, it is no longer necessary to have one Xargo.toml for each Cargo.toml, but only one next to the workspace, for example.

Co-authored-by: Nils Asmussen <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 19, 2019

Build succeeded

@bors bors bot merged commit a5f8503 into japaric:master Sep 19, 2019
@RalfJung
Copy link
Collaborator

Yay, finally. :)

@Nils-TUD
Copy link
Contributor Author

Great :)

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