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

Experiment with a cttests_macro proc_macro. #416

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

ratmice
Copy link
Collaborator

@ratmice ratmice commented Aug 30, 2023

This was just a quick experiment with generating tests which parse cttest/src/*.test from a proc_macro to generate #[test] fragments.

  1. Moving the cttests/build.rs helper functions to a file src/cgen_helper.rs
  2. Adds a cttests_macro crate (doesn't need to live on crates.io, used as a path dependency)
  3. Uses that macro to generate #[should_panic] tests calling the cgen_helper functions.

In #415 I know you said it probably would not be worth the effort, but I was curious if it would actually work.
You had mentioned using the lang_tester crate. But I'm just uncertain of how to go about that given the build.rs involvement and (moderate) dependence on cargo environment variables like OUT_DIR within the CTBuilders.

Not really sure if all the additional complexity is worthwhile however at least I learned some proc_macro things.

@ltratt
Copy link
Member

ltratt commented Aug 30, 2023

Does this mean that the tests are always run now or ... ?

@ratmice
Copy link
Collaborator Author

ratmice commented Aug 30, 2023

Yeah, the additions to lrpar/src/lib.rs causes it to generate a bunch of #[test] functions that run along side all of the other #[tests]. So, even if the build.rs is never rebuilt these tests will now run when you cargo test.

There is a few things in this which i should probably investigate further, regarding the globbing/lossy string conversion which might cause an issue if someone uses a non-lossy convertible character in their home directory since the manifest path is absolute.
But it's a basic functioning proof of concept at how the proc-macro approach could work.

@ltratt
Copy link
Member

ltratt commented Aug 30, 2023

Got it. I'm happy with this: do you want to do some of the tidying up you suggested before merge?

@ratmice
Copy link
Collaborator Author

ratmice commented Aug 31, 2023

I would like to tidy it up, I don't think the current master branch is affected by it so it likely is introducing a top-level cargo test regression, which could affect users with non-utf8 home directories.

It looks like both glob and globset have unresolved issues regarding this,

rust-lang/glob#78
BurntSushi/ripgrep#1250

In theory we can avoid non-utf8 characters by building a relative path, but it is complicated by proc_macros not having a specified working directory. Assuming it is in the workspace somewhere, we should be able to strip the common prefix between the working dir, and $CARGO_MANIFEST_DIR.

A bit hacky, but I think that is the only option short of fixing glob.
Edit: I guess we could also cd $CARGO_MANIFEST_PATH && glob(...) && cd -, while that would be simplest and avoids any string manipulation it seems like it isn't thread safe, I would probably rather avoid it even though it doesn't appear that the compiler is multi-threaded during proc_macro evaluation at the moment 🤷

@ltratt
Copy link
Member

ltratt commented Aug 31, 2023

Presumably we haven't made the non-utf8 situation worse in this PR?

@ratmice
Copy link
Collaborator Author

ratmice commented Aug 31, 2023

Presumably we haven't made the non-utf8 situation worse in this PR?

It has or would, currently we only deal with known utf8 paths relative to the working dir.
The code in this branch has to deal with 2 conversions, one in and one out of glob. The one out of glob being
conversion of the path to tokens for the macro return value (I.e. there isn't a valid rust source code string which corresponds to all valid paths).

Previously we had known utf8 input, and never need to convert the output so it should be fine currently.

@ratmice
Copy link
Collaborator Author

ratmice commented Aug 31, 2023

I did my best to fix it in the obvious issues in that last patch, but i'm not really sure how to go about creating non-utf8 directory to test. I think though it should put us back on par with the current code on the master branch.

@ratmice ratmice marked this pull request as ready for review August 31, 2023 08:52
@ltratt
Copy link
Member

ltratt commented Aug 31, 2023

I think this comes under "if the fix doesn't work for some edge cases, we'll deal with it when we find it"!

Please squash.

@ratmice
Copy link
Collaborator Author

ratmice commented Aug 31, 2023

Squashed.

@ltratt
Copy link
Member

ltratt commented Aug 31, 2023

bors r+

bors bot added a commit that referenced this pull request Aug 31, 2023
416: Experiment with a cttests_macro proc_macro. r=ltratt a=ratmice



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

bors bot commented Aug 31, 2023

Build failed:

@ratmice
Copy link
Collaborator Author

ratmice commented Aug 31, 2023

Oops, above commit msg should be rustfmt, but given it'll be squashed away i'll leave it for now.
Build failure should be fixed I believe.

@ltratt
Copy link
Member

ltratt commented Aug 31, 2023

Please squash.

@ratmice
Copy link
Collaborator Author

ratmice commented Aug 31, 2023

Sorry. One last change to fix clippy.

@ltratt
Copy link
Member

ltratt commented Aug 31, 2023

Please squash.

@ratmice
Copy link
Collaborator Author

ratmice commented Aug 31, 2023

Squashed -- hopefully for the last time!

@ltratt
Copy link
Member

ltratt commented Aug 31, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 31, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit b1111d9 into softdevteam:master Aug 31, 2023
@ratmice ratmice deleted the cttests_macro branch August 31, 2023 12:50
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