Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add CI #10

Merged
merged 3 commits into from
Oct 18, 2022
Merged

Add CI #10

merged 3 commits into from
Oct 18, 2022

Conversation

eldruin
Copy link
Member

@eldruin eldruin commented Aug 31, 2022

I noticed one failing test for snprintf.
As the original author, is that something you could look into @thejpster?

Fixes #9

@jonathanpallant
Copy link
Collaborator

I think the test matrix is wrong. You can only pass the lp32 feature on platforms where Long and Pointers are 32-bit, which is not true on x86_64-unknown-linux-gnu. The difference in length is cause because -100i32 is passed to sprintf("%ld") and -100i32 isn't a long, and the wrong number of bytes is read giving a bad (much longer) string output.

@eldruin
Copy link
Member Author

eldruin commented Sep 1, 2022

I see. Do you know any architecture available in cross that I can use to run the tests for LP32? What about the no-features case?

@jonathanpallant
Copy link
Collaborator

i686-something would be LP32 targets. You can target that on an x86-64 machine without using cross, I believe.

@jonathanpallant
Copy link
Collaborator

(I mean, you can /target/ anything but of course the point of the tests is to execute them, and an x86-64 machine will execute i686 code just fine).

@jonathanpallant
Copy link
Collaborator

I can't remember what happens with no features, and I'm afraid I'm not in a position to check right now. I seem to recall the code only checks LP64 and not(LP64) and only when looking at %ld for snprintf, but I could be wrong.

@thejpster
Copy link
Member

Can you rebase this once the other PR is in? Maybe our issues will go away.

@eldruin eldruin force-pushed the master branch 3 times, most recently from f55e064 to 58dcc3f Compare October 18, 2022 06:28
@eldruin
Copy link
Member Author

eldruin commented Oct 18, 2022

They are indeed! This can now be reviewed and merged.

@thejpster thejpster merged commit a1c832b into rust-embedded-community:master Oct 18, 2022
@thejpster
Copy link
Member

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CI
3 participants