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

Tweak Rust package guidelines #670

Closed
alerque opened this issue Jan 31, 2022 · 7 comments
Closed

Tweak Rust package guidelines #670

alerque opened this issue Jan 31, 2022 · 7 comments

Comments

@alerque
Copy link

alerque commented Jan 31, 2022

I don't have any RISC boards, but poking around in here one thing that seems to need patching a lot is something I put in the Rust packaging guidelines.

https://github.com/felixonmars/archriscv-packages/blob/82d8d619e862478a076ae657069c9caf2f1caed4/onefetch/riscv64.patch

This platform limited fetch can mean a huge savings in downloads when building packages (in some cases from hundreds of MB down to less than 10), so I am loath to drop it altogether.

Would it be helpful to modify the Rust recommendations like so:

- cargo fetch --target "$CARCH-unknown-linux-gnu"
+ cargo fetch --target "$CHOST"

...or is the issue here that you are cross compiling and need to fetch far an architecture other than the host? If so is there a variable we could suggest people use to get the benefit of only downloading required sources while not making so much work patching downstream?

@r-value
Copy link
Collaborator

r-value commented Jan 31, 2022

Unfortunately, neither $CARCH nor $CHOST provides correct triple for Rust. Rust needs something like riscv64gc but these variables are riscv64. So we'd like to just trim them off.

Ref: https://github.com/felixonmars/archriscv-packages/blob/master/devtools-riscv64/makepkg-riscv64.conf#L37

@alerque
Copy link
Author

alerque commented Jan 31, 2022

Bummer. Also a bummer this never got flushed out. Also a bummer my patch fixing the cargo fetch --target documentation that fixes the current contradiction in the man page was dropped. I need to resubmit that at least...

So why is Rust coming up with a different architecture name? Is this something it's impossible to normalize?

@XieJiSS
Copy link
Collaborator

XieJiSS commented Jan 31, 2022

Hi @alerque ,

https://github.com/laanwj/cc-rs/blob/4981b3cdf070c5f1f8ead508d5b56ea60ec657c2/src/lib.rs#L1431-L1441

I think that's partially because the compiler need to construct the -march argument to be passed to the underlying clang/llvm stuff from the --target argument. The -march uses the format similar to rv64gc / rv64imac.

@r-value
Copy link
Collaborator

r-value commented Jan 31, 2022

Is this something it's impossible to normalize?

I think that there can be two possible solutions. One is submitting patch to Rust and change the naming scheme to something like arch=riscv64 and put extensions like G/C/V/K in target-feature just like what they did with x86 extensions. The other is to make a new environment variable like $RSARCH which contains riscv64gc on RISC-V and x86_64 on x86.

@alerque
Copy link
Author

alerque commented Jan 31, 2022

Bah. So basically right now the only way to solve both my concern and eliminate the need for downstream patching would be something like this:

cargo fetch --locked --target "$(rustc -vV | awk '/^host:/ { print $2 }')"

Obviously that's a bit convoluted so I'm not recommending it yet, just trying to get my head around the problem here.

It seems to me the best course of action would be to actually fix cargo to work as documented:

--target triple
Fetch for the given architecture. The default is the host architecture.

As far as I can see there is no way to actually use this so called "default" in fetch, it only defaults to "all architectures" (which for some dependency chains is becoming pretty heavy!).

@piggynl
Copy link
Contributor

piggynl commented Feb 17, 2022

Hi @alerque, I recreated your PR for cargo's doc change as rust-lang/cargo#10398 👀

Avimitin added a commit to Avimitin/archriscv-packages that referenced this issue Apr 21, 2022
This patch fix three error:

1. The [target specification issue].

2. Crate libc can not be compiled due to symbol missing.

This is fixed in [rust-lang/libc PR felixonmars#2668], and release in 0.2.118
version.

3. Test failure

Some tests of diskonaut depend on the previous events. It take a
snapshot of the TUI pixels output, then compare those snapshot pixel by
pixel to determine if the cli is working correctly.

However those test are not reliable. If the previous event took too
long to finish, the TUI output will not match what is expected.

The current workaround is to increase the event interval. One second
interval is enough for all event to finish its jobs.

Tested in 5950x QEMU environment.

Ref:
* [target specification issue]: felixonmars#670 (comment)
* [rust-lang/libc PR felixonmars#2668]: rust-lang/libc#2668

Signed-off-by: Avimitin <avimitin@gmail.com>
felixonmars pushed a commit that referenced this issue Apr 22, 2022
This patch fix three error:

1. The [target specification issue].

2. Crate libc can not be compiled due to symbol missing.

This is fixed in [rust-lang/libc PR #2668], and release in 0.2.118
version.

3. Test failure

Some tests of diskonaut depend on the previous events. It take a
snapshot of the TUI pixels output, then compare those snapshot pixel by
pixel to determine if the cli is working correctly.

However those test are not reliable. If the previous event took too
long to finish, the TUI output will not match what is expected.

The current workaround is to increase the event interval. One second
interval is enough for all event to finish its jobs.

Tested in 5950x QEMU environment.

Ref:
* [target specification issue]: #670 (comment)
* [rust-lang/libc PR #2668]: rust-lang/libc#2668

Signed-off-by: Avimitin <avimitin@gmail.com>
@Xeonacid
Copy link
Collaborator

Closing since there has been a working Rust package guidelines for a while. :)

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

5 participants