201/pass cfg flags #214

NathanFrasier · 2023-08-07T02:14:12Z

This is a little rough. I feel like there has to be a better way to get rid of some of the calls to .push() and .extend() but I wasn't thinking of them.

This still needs documentation as well.

Addresses #201

cargo-marker/src/backend.rs

marker_adapter/src/loader.rs

xFrednet

This is a nice start. I haven't tested the cfg part myself yet, but believe that this will work, based on your comments in the issue.

For testing the environment value, I can recommend using the --test-setup argument for dogfood. It'll print out the environment values, instead of running marker, after everything is compiled. For me, it roughly looked like this:

$ cargo dogfood --test-setup
# [...]
env:RUSTC_WORKSPACE_WRAPPER=.../marker/target/debug/marker_rustc_driver
env:MARKER_LINT_CRATES=marker_lints:.../marker/target/marker/lints/libmarker_lints.so;marker_uilints:.../marker/target/marker/lints/libmarker_uilints.so
info:toolchain=nightly-2023-07-13
info:marker-api=0.1.1

(With #213 merged it will become cargo dogfood test-setup).

I left some comments, as always, please don't hesitate to ask if you have any questions.

cargo-marker/src/backend/lints/build.rs

xFrednet · 2023-08-07T09:52:05Z

cargo-marker/src/backend/lints/build.rs

    for lint_src in sources {
        build_lint(lint_src, config)?;
-    }
-
-    // Find lint lint crates
-    match std::fs::read_dir(&lints_dir) {
-        Ok(dir) => {
-            let ending = OsStr::new(DYNAMIC_LIB_FILE_ENDING);
-            let mut lints = vec![];
-            for file in dir {
-                let file = file.unwrap().path();
-                if file.extension() == Some(ending) {
-                    lints.push(LintCrate { file });
+        match std::fs::read_dir(&lints_dir) {


As you noted above, it feels inefficient to scan the directory after every compilation. cargo metadata sadly doesn't provide any information how the build file will be called.

I see a few solutions:

Leaving it like this, it seems to work

Your suggestion of changing the target directory per crate

Guess the output file name and error if the name can't be found. The dynamic library names are constructed by adding a lib prefix on unix systems and the OS-specific file ending. Here is a draft of the code:

// Probably store these above as constants with `DYNAMIC_LIB_FILE_ENDING` #[cfg(unix)] let lib_file_prefix = "lib"; #[cfg(windows)] let lib_file_prefix = ""; // ... let dir = &lints_dir; for lint_src in sources { build_lint(lint_src, config)?; let name = lint_src.name.clone(); let file = dir.join(format!("{lib_file_prefix}{name}.{DYNAMIC_LIB_FILE_ENDING}")); if file.exists() { lints.push(LintCrate { name, file }); } }

Guess the file name with more confidence. Option 3 would fail, if the user specified a name field in the [lib] section, as that changes the name. This is where we can do some more hackery:

Cargo's build command supports the --config KEY=VALUE argument, to override parts of the manifest (At least from my understanding). We should be able to use this during lint compilation to prevent any weird renames, by setting the name ourself: --config "lib.name = '<lint_crate_name>'". Then we can just apply option 3 to confidently guess that the file actually exists and uses the name we expect.

Would you mind looking at option 4? It's totally fine if not, this is already a really nice addition to Marker. Option 2, 3 and 4 can also be added later :)

I can absolutely look into number 4, But I'd like to focus on getting this in and the user documentation ready first. I'm inclined to handle making this path more efficient as a separate issue, but If you think it should be part of this, I can work on it here.

I was trying to think if we'd get any real sandboxing type benefits from option 2, but I don't really see a point in making sure that some lints can't use build artifacts of others. it would just be more file system usage with little benefit. Besides, if there's one thing I learned writing C#, it's that windows doesn't like deeply nested files, so don't make folders unless you need them :)

For me it's fine if we continue this implementation and do the path cleanup in a later PR. This system should have the same robustness level :)

marker_adapter/src/loader.rs

marker_rustc_driver/src/main.rs

xFrednet · 2023-08-07T21:34:44Z

Btw, can you also add a test case in the uilints crate?

The CI checks that the cargo-marker crate builds on stable, we can use this to test that #[cfg_atte(marker, allow(...))] allows crates to build on stable

xFrednet

This version looks good to me, I have three comments, which will hopefully be simple. Thank you for writing the documentation!

xFrednet · 2023-08-08T11:16:44Z

docs/book/src/usage.md

@@ -1,3 +1,29 @@
 # Usage

 This section is intended for users who want to install Marker and run provided lints.
+
+## Controlling Marker Lints


Can you move this into a separate file/section in the usage folder? (Similar to #212)

And maybe Setting Lint Levels would be a more descriptive title

marker_uilints/tests/ui/cfg_attr_check.rs

xFrednet · 2023-08-09T11:16:13Z

docs/book/src/usage.md

+Marker provides this functionality via the `#[register_tool()]` attribute currently available in
+[nightly](https://github.com/rust-lang/rust/issues/66079). If your crate is compiled using nightly, then controlling
+lints is as simple as placing `#![feature(register_tool)]` and `#![register_tool(marker)]` at the top of your crate
+`lib.rs` or `mod.rs` file. You can then use all of the normal lint attributes you might usually use, on marker provided
+lints, like `#[allow(marker::my_lint)]`.


I would make this statement a bit stronger, saying that rustc requires the feature. Maybe:

Marker's lints use the `marker::` tool prefix. Rustc requires the [`register_tool` feature](https://github.com/rust-lang/rust/issues/66079) to allow this prefix in the normal lint level attributes, like `#[allow(...)]` and `#[deny(...)]`....

Marker's lints use the marker:: tool prefix. Rustc requires the register_tool feature to allow this prefix in the normal lint level attributes, like #[allow(...)] and #[deny(...)]....

It's likely that most projects use a stable version by default. It would be nice to have a small code block, which could be directly copied. This should probably work:

#[cfg_attr(marker, feature(register_tool))] #[cfg_attr(marker, register_tool(marker))]

NathanFrasier · 2023-08-15T23:26:53Z

@xFrednet I know I've been absent from the project for a minute. I'm going on vacation after this week and would like to get this in, rather than leave you hanging waiting on the few changes left on here. I plan to rebase to current and make the changes mentioned above, is there anything else? I saw you self-assigned a few days ago.

xFrednet · 2023-08-15T23:55:18Z

Hey @NathanFrasier, no problem at all, thank you for coming back and wanting to finish it! This week, I'm also taking it a bit easy. For the PR, only the documentation and test update were left. I'll review your rebased changes tomorrow. The self assignment, was just to give me an overview :)

… key-value map with crate names

xFrednet

Thank you! This version looks perfect, I just have three small remarks about the docs. Do you have the time to push an update before your vacation? Otherwise, I'm more than happy to merge this and add them on master. The implementation is really solid, and I'm looking forward to including this in v0.2.0

docs/book/src/usage/setting-lint-levels.md

xFrednet

This version looks excellent, thank you for working on this issue and finishing the PR!

Enjoy your vacation! ⛱️

bors r+

bors · 2023-08-16T20:58:50Z

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.

NathanFrasier commented Aug 7, 2023

View reviewed changes

cargo-marker/src/backend.rs Outdated Show resolved Hide resolved

NathanFrasier commented Aug 7, 2023

View reviewed changes

marker_adapter/src/loader.rs Show resolved Hide resolved

xFrednet added C-enhancement Category: New feature or request A-marker-adapter Area: Adapter A-marker-cargo Area: All things connected to `cargo_marker` S-waiting-on-review Status: Awaiting review labels Aug 7, 2023

xFrednet added this to the v0.2.0 milestone Aug 7, 2023

xFrednet reviewed Aug 7, 2023

View reviewed changes

NathanFrasier marked this pull request as ready for review August 8, 2023 02:43

xFrednet reviewed Aug 9, 2023

View reviewed changes

xFrednet self-assigned this Aug 12, 2023

xFrednet mentioned this pull request Aug 12, 2023

Fixed toolchain version drift, improved error handling, config loading and refactoring. #217

Merged

NathanFrasier force-pushed the 201/pass-cfg-flags branch from bd9d3c6 to 793ff79 Compare August 15, 2023 23:28

NathanFrasier added 3 commits August 15, 2023 19:33

Marker, Adapter: Change the MARKER_LINT_CRATES env variable to pass a…

6de93a8

… key-value map with crate names

rustc: Pass crate names in a marker cfg flag

a74ab7c

CI: add cfg flag test

702d58a

NathanFrasier force-pushed the 201/pass-cfg-flags branch from 58abfd1 to 3977556 Compare August 16, 2023 00:33

xFrednet reviewed Aug 16, 2023

View reviewed changes

docs/book/src/usage/setting-lint-levels.md Outdated Show resolved Hide resolved

docs/book/src/usage/setting-lint-levels.md Outdated Show resolved Hide resolved

docs/book/src/usage/setting-lint-levels.md Outdated Show resolved Hide resolved

NathanFrasier force-pushed the 201/pass-cfg-flags branch from 3977556 to 77993c1 Compare August 16, 2023 17:22

Doc: add documentation covering usage of the new --cfg flags

1fb3b1e

NathanFrasier force-pushed the 201/pass-cfg-flags branch from 77993c1 to 1fb3b1e Compare August 16, 2023 17:24

xFrednet approved these changes Aug 16, 2023

View reviewed changes

bors bot merged commit dc477cf into rust-marker:master Aug 16, 2023
6 checks passed

xFrednet mentioned this pull request Aug 18, 2023

Add a way to set the lint level on the function/statement level, without the register_tool feature #201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

201/pass cfg flags #214

201/pass cfg flags #214

NathanFrasier commented Aug 7, 2023 •

edited

Loading

xFrednet left a comment

xFrednet Aug 7, 2023

NathanFrasier Aug 7, 2023

xFrednet Aug 7, 2023

xFrednet commented Aug 7, 2023

xFrednet left a comment

xFrednet Aug 8, 2023

xFrednet Aug 9, 2023

NathanFrasier commented Aug 15, 2023

xFrednet commented Aug 15, 2023

xFrednet left a comment

xFrednet left a comment

bors bot commented Aug 16, 2023

201/pass cfg flags #214

201/pass cfg flags #214

Conversation

NathanFrasier commented Aug 7, 2023 • edited Loading

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Aug 7, 2023

Choose a reason for hiding this comment

NathanFrasier Aug 7, 2023

Choose a reason for hiding this comment

xFrednet Aug 7, 2023

Choose a reason for hiding this comment

xFrednet commented Aug 7, 2023

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Aug 8, 2023

Choose a reason for hiding this comment

xFrednet Aug 9, 2023

Choose a reason for hiding this comment

NathanFrasier commented Aug 15, 2023

xFrednet commented Aug 15, 2023

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

bors bot commented Aug 16, 2023

NathanFrasier commented Aug 7, 2023 •

edited

Loading