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

marker_rustc_driver as a library? #257

Closed
smoelius opened this issue Sep 25, 2023 · 5 comments · Fixed by #271
Closed

marker_rustc_driver as a library? #257

smoelius opened this issue Sep 25, 2023 · 5 comments · Fixed by #271
Labels
C-discussion Category: Discussion / Design decision. See also rust-marker/design D-dylint Driver: Dylint with marker as a lint

Comments

@smoelius
Copy link
Contributor

How would you feel about exposing marker_rustc_driver internals as a library?

Context: I have been experimenting with idea of running Marker lints under a Dylint driver, and I have a PoC. The PoC is essentially a single configurable Dylint library. The configuration (i.e., dylint.toml file) tells the Dylint library where to find the Marker libraries. For example, in my tests, the file looks as follows:

[[marker.lint_crates]]
name = "marker_lints"
path = "/tmp/.../target/marker/lints/libmarker_lints.so"

So, essentially, the PoC is a shared library that loads other shared libraries. For anyone interested in trying it out, the following should work:

git clone https://github.com/smoelius/dylint --branch marker
cd dylint/examples/testing/marker
cargo test

This will run a slightly modified version of the diag_msg_uppercase_start tests using the setup described above.


To make the above work, I had to copy a bunch of marker_rustc_driver's code. I would like to include this PoC as a test in the Dylint repository, but I would prefer to not copy that code (hence, this question).

Note that GitHub access to the library would suffice. So if you do not want to publish marker_rustc_driver internals as a library on crates.io (which would be perfectly reasonable), there may be a way to expose the internals as a library through conditional compilation.

@smoelius smoelius added the S-needs-triage Status: This issue needs triage label Sep 25, 2023
@xFrednet xFrednet added D-dylint Driver: Dylint with marker as a lint C-discussion Category: Discussion / Design decision. See also rust-marker/design and removed S-needs-triage Status: This issue needs triage labels Sep 26, 2023
@xFrednet
Copy link
Member

When we discussed this topic previously, I didn't think about the fact that marker_rustc_driver is only a binary. From the technical POV, I'm fine with making it a library, if it's as easy as I'm currently expecting and you described. I have two questions, though:

  1. Marker is bound to a specific nightly version. AFAIK Dylint was usually very flexible in this regard, how do you intend to handle this, if you include Marker as a Dylint?
  2. Marker receives an update every 6 weeks, when a new Rust version is released, how do you intend to update Marker for Dylint?

I want to explain where I got these questions from. Since Dylint has a larger ordinance right now, it would be cool to get some more exposure this way. However, if the provided Marker version in Dylint causes compilation errors due to wrong nightly versions or is very outdated, I fear that it might give a negative first impression. I fully believe in your good intentions, which is why I state this concern so openly. I hope you understand why I'm asking these questions. :)

@smoelius
Copy link
Contributor Author

From the technical POV, I'm fine with making it a library

Cool. I'd be happy to submit a PR. Is it okay to add a lib target to marker_rustc_driver (which would then appear on crates.io)? Or would you prefer to go the conditional compilation route?

  1. Marker is bound to a specific nightly version. AFAIK Dylint was usually very flexible in this regard, how do you intend to handle this, if you include Marker as a Dylint?
  2. Marker receives an update every 6 weeks, when a new Rust version is released, how do you intend to update Marker for Dylint?

The PoC is currently pinned to the toolchain Marker 0.2.1 uses (nightly-2023-07-13).

I expect it would not be hard to write a GitHub workflow to open a PR whenever there is a new Marker release. That PR could include an update to the Dylint library's rust-toolchain file, i.e., to version the new Marker release uses.

So, in other words, these two things should stay in sync:

  • the toolchain used by the Dylint library
  • the toolchain used by the Marker release used by the Dylint library

@xFrednet
Copy link
Member

Or would you prefer to go the conditional compilation route?

Hmm, I'd prefer it being a conditional target if that's easily possible. An advantage of this change is that marker_rustc_driver would probably be documented on doc.rs (Binaries are ignored by default)

@Veetaha Do you think, such a change would conflict with the release infrastructure you build for Marker?

  • the toolchain used by the Dylint library
  • the toolchain used by the Marker release used by the Dylint library

Exactly, the question is basically if you're okay with taking up the responsibility of keeping them in sync.


Small side note: My RL is currently taking up a good amount of my time, and I'm focussing the next release. It might take a bit more time until I respond to the Dylint discussion, but that's due to external factors :)

@Veetaha
Copy link
Contributor

Veetaha commented Sep 27, 2023

Do you think, such a change would conflict with the release infrastructure you build for Marker?

It don't, marker_rustc_driver is published to crates.io regardless of wether it has a library target or not, and the build command stays the same, it should still work.
Although I don't understand how you could make a conditionally compiled library target, and, moreover, make it opt-in such that the current build flow wouldn't change.
If that turns out to be too complicated I think we could do a lib target non-conditionally. I doubt that anyone would ever want to depend on marker_rustc_driver without realizing that its interface is subject to change outside of semver guarantees

@smoelius
Copy link
Contributor Author

The simplest solution would be to use an unconditional library target. I will submit a PR using that approach. If it's deemed undesirable, I will investigate the conditional compilation idea.

Exactly, the question is basically if you're okay with taking up the responsibility of keeping them in sync.

I am okay with it. (I expect that part to be easy.)

Small side note: My RL is currently taking up a good amount of my time, and I'm focussing the next release. It might take a bit more time until I respond to the Dylint discussion, but that's due to external factors :)

No problem at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion / Design decision. See also rust-marker/design D-dylint Driver: Dylint with marker as a lint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants