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

feat: runtime test expectation file #70

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat: runtime test expectation file #70

wants to merge 10 commits into from

Conversation

Gankra
Copy link
Owner

@Gankra Gankra commented Jul 13, 2024

fixes #9

src/cli.rs Outdated Show resolved Hide resolved
busted = "build"

# CI GCC is too old to support _Float16
[targets.x86_64-unknown-linux-gnu."f16::conv_c"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Having some way to support arbitrary cfg() conditions would be really useful to indicate that something is broken on a certain architecture or OS without having to list every single target with this architecture or OS you want to test.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm yeah I agree, but am definitely running out of steam to build out this feature myself, and am hoping this is an acceptable MVP X3

A few things:

  • I specifically made it so you can do [targets."*"."blhablhablha"] if you really don't care
  • I expect most users are going to be copy-pasting what the new suggestion system spits out, so verbosity is hopefully less of an issue?
    • And having "too many" records makes it easier to granularly update them if one platform is fixed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Partially there is some sense of like, when bringing up a new platform, I think you do want some expectations tabula-rasa? But I might be wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have a test failing on x86_64-unknown-linux-gnu, it is almost guaranteed to fail on all x86_64 unix targets as those share a single abi document. The only cases I know of where the OS actually matters are unix vs windows-msvc vs windows-gnu(?) on x86_64 and (non-apple) unix vs apple vs windows arm64ec on arm64.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm are there any libraries y'all compiler devs have for evaling this stuff at runtime?

Copy link
Contributor

@bjorn3 bjorn3 Jul 14, 2024

Choose a reason for hiding this comment

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

You could try using the target-lexicon crate (as used by cranelift and by extension cg_clif). It has a default_calling_convention method which returns the calling convention used for extern "C" (also mostly applies to extern "Rust") on the respective target. It doesn't yet handle arm64ec though.

@Gankra Gankra marked this pull request as ready for review July 14, 2024 16:15
@Gankra
Copy link
Owner Author

Gankra commented Jul 14, 2024

Ok, this is a minimal working implementation of everything. On error you now get output like this:

image

@Gankra
Copy link
Owner Author

Gankra commented Jul 14, 2024

Also note that the suggester is really limited, in that I didn't want to teach it to edit/remove/merge rules. So it always just suggests new rules to concatenate at the end (which can actually be the correct thing to do since the rules get applied in a specific order, so you can get a kind of specialization/css thing going on with specific later rules overriding broader earlier ones.

@Gankra
Copy link
Owner Author

Gankra commented Jul 14, 2024

I'm gonna hold off on merging this until @bjorn3 is happy with it, since this is gonna make a bunch of work for 'em, and it's kinda "for" 'em.

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.

Add proper out-of-line test expectation files
2 participants