cargo-marker: add git support #126

Niki4tap · 2023-04-20T19:51:56Z

Its been a while since I've worked on this, but I've finally took the time to finish it.

This PR adds git support to marker so this now works:

[workspace.metadata.marker.lints]
marker_lints = { git = "https://github.com/rust-marker/marker" }

It makes use of cargo_fetch library which I made for this specific use-case, which is essentially just a thin wrapper around cargo library (so the builds are now way slower :p).

I'll try to maintain the library to my best capacity, and prioritize marker's goals whenever needed.

Addition of the library also technically allows for any kind of package to be downloaded, so this also technically supports registries, though this PR does not implement it, because this is blocked on marker APIs being released.

In total this checks out... *looks up the issue*... one task from #81!
But also technically closes #87 whenever APIs are published.

review side note: most changes are from Cargo.lock

xFrednet · 2023-04-21T16:01:52Z

This is awesome, I was super happy when I saw the notification! I haven't taken a look at it yet, I'll do so next week :)

xFrednet · 2023-04-21T16:04:53Z

It makes use of cargo_fetch library which I made for this specific use-case

That is super cool! I'm not 100% sure if git dependencies work for crates we want to push to crates.io. But if that becomes a problem, we could move it to here or publish it. Let me know if you need any help with the maintenance!

Niki4tap · 2023-04-21T16:57:13Z

That is super cool! I'm not 100% sure if git dependencies work for crates we want to push to crates.io

Oh I know, that's why I've published it ;)

xFrednet

I'm honestly impressed. Thank you very much for all the work you put into this, even making a new library. This is so much more than I hoped!

I still want to do some small tests before merging it, but currently everything looks, awesome!

Btw, do you have a lint/repo, that you tested this implementation with? Otherwise, I might look into adding a repo to rust-marker and hacking a test together :)

xFrednet · 2023-04-25T16:06:33Z

Btw, let's make sure it runs everywhere:

bors try

bors · 2023-04-25T16:19:49Z

try

Build succeeded:

Niki4tap · 2023-04-25T16:24:33Z

Btw, do you have a lint/repo, that you tested this implementation with? Otherwise, I might look into adding a repo to rust-marker and hacking a test together :)

Yeah some tests would be great, but yes, I've tested locally with the marker repo itself, you can just do:

[workspace.metadata.marker.lints]
marker_lints = { git = "https://github.com/rust-marker/marker" }

in the Cargo.toml, and it should fetch the test lints from the repo

xFrednet · 2023-05-05T19:12:06Z

Hey, I finally found some time to test this. Sorry, for my inactivity. My semester will end in about one month and then I'll have more time, also to (hopefully) make the final push for v0.0.1.

Anyways, I see what you mean, that the compile time increased significantly. In the future, I would be in favor of having such a functionality in Cargo itself, there is already an issue for that. However, in the meantime, this is the best solution I can think of, and I'm really grateful that you created the library ❤️

Now, related to testing:

I've reserved the marker_lint_crate_test crate name and would like to later include a fetch test for both git and crates.io in the CI. However, having a test crate for that (besides marker_lints) sounds a bit painful, until we have a working template and an idea of the stable API. AKA v0.0.1. I crated #127 to create a template create we can use for testing, once we reach that point. Until then, I think we should merge this without CI tests.

I've tested the implementation locally. The git fetching works 🎉 However, the normal path dependency:

[workspace.metadata.marker.lints]
marker_lints = { path = "marker_lints" }

is sadly broken. I get the following error when running cargo dogfood on this branch. (Without any further modifications)

$ c clean
# (Yes, I have an alias cargo, and I can highly recommend it xD)
$ c dogfood
   Compiling proc-macro2 v1.0.49
   [..]
   Compiling cargo_marker v0.1.0 (./marker/cargo-marker)
    Finished dev [unoptimized + debuginfo] target(s) in 59.85s
     Running `target/debug/cargo-marker --forward-rust-flags`
Compiling rustc driver
   Compiling proc-macro2 v1.0.49
   [...]
   Compiling marker_driver_rustc v0.1.0 (./marker/marker_driver_rustc)
    Finished dev [unoptimized + debuginfo] target(s) in 7.38s

Compiling Lints:
Failed fetching lint crates: invalid path url `marker_lints`
Error: LintCrateFetchFailed

Could you take a look at this? :) Once that is fixed, the PR is ready to be merged.

Niki4tap · 2023-05-05T23:44:23Z

Hey, I finally found some time to test this. Sorry, for my inactivity.

No problem, I'm not rushing, I have other things to deal with myself.

Anyways, I see what you mean, that the compile time increased significantly. In the future, I would be in favor of having such a functionality in Cargo itself, there is already an issue for that.

Yeah the library seems like a hacky solution, and I would like to see it in the cargo too, but this works for now :p

However, the normal path dependency is sadly broken.

Nice catch, funny enough it was a typo (PackageSource::Path the enum variant vs PackageSource::path the helper method), rust may protect you from memory errors, but not from typos I guess 😄

Everything should be good now I think

xFrednet

Alright, this version looks good to me. Thank you!!!

bors r+

126: cargo-marker: add git support r=xFrednet a=Niki4tap Its been a while since I've worked on this, but I've finally took the time to finish it. This PR adds git support to marker so this now works: ```toml [workspace.metadata.marker.lints] marker_lints = { git = "https://github.com/rust-marker/marker" } ``` It makes use of [`cargo_fetch`](https://github.com/Niki4tap/cargo_fetch) library which I made for this specific use-case, which is essentially just a thin wrapper around `cargo` library (so the builds are now way slower :p). I'll try to maintain the library to my best capacity, and prioritize marker's goals whenever needed. Addition of the library also technically allows for any kind of package to be downloaded, so this also *technically* supports registries, though this PR does not implement it, because this is blocked on marker APIs being released. --- In total this checks out... *\*looks up the issue\**... one task from #81! But also *technically* closes  #87 whenever APIs are published. review side note: most changes are from `Cargo.lock` Co-authored-by: Niki4tap <rombiklol2@gmail.com>

bors · 2023-05-07T09:18:23Z

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

Response status code: 422
{"message":"Validation Failed","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

xFrednet · 2023-05-07T09:20:29Z

Looks like my tests with merge queues broke bors 😅

Now it should work again

bors r+

bors · 2023-05-07T09:31:06Z

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.

Niki4tap requested a review from xFrednet April 20, 2023 19:52

Niki4tap added C-enhancement Category: New feature or request A-marker-cargo Area: All things connected to `cargo_marker` S-waiting-on-review Status: Awaiting review labels Apr 20, 2023

Niki4tap force-pushed the cargo-marker-git branch from 1a6c420 to 00a363a Compare April 21, 2023 19:14

xFrednet approved these changes Apr 25, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Apr 25, 2023

Try #126:

d0ff396

xFrednet self-assigned this May 5, 2023

Initial git support

4c23ddd

Niki4tap force-pushed the cargo-marker-git branch from 00a363a to 4c23ddd Compare May 5, 2023 23:34

xFrednet approved these changes May 7, 2023

View reviewed changes

xFrednet mentioned this pull request May 7, 2023

Allow the specification of lint crates in a file (local path and git) #81

Closed

bors bot merged commit bb02eb1 into rust-marker:master May 7, 2023

xFrednet mentioned this pull request Jul 10, 2023

Migrate away from cargo_fetch and bind to cargo directly #167

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo-marker: add git support #126

cargo-marker: add git support #126

Niki4tap commented Apr 20, 2023 •

edited

Loading

xFrednet commented Apr 21, 2023

xFrednet commented Apr 21, 2023

Niki4tap commented Apr 21, 2023

xFrednet left a comment

xFrednet commented Apr 25, 2023

bors bot commented Apr 25, 2023

Niki4tap commented Apr 25, 2023

xFrednet commented May 5, 2023

Niki4tap commented May 5, 2023

xFrednet left a comment

bors bot commented May 7, 2023

xFrednet commented May 7, 2023

bors bot commented May 7, 2023

cargo-marker: add git support #126

cargo-marker: add git support #126

Conversation

Niki4tap commented Apr 20, 2023 • edited Loading

xFrednet commented Apr 21, 2023

xFrednet commented Apr 21, 2023

Niki4tap commented Apr 21, 2023

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet commented Apr 25, 2023

bors bot commented Apr 25, 2023

try

Niki4tap commented Apr 25, 2023

xFrednet commented May 5, 2023

Niki4tap commented May 5, 2023

xFrednet left a comment

Choose a reason for hiding this comment

bors bot commented May 7, 2023

xFrednet commented May 7, 2023

bors bot commented May 7, 2023

Niki4tap commented Apr 20, 2023 •

edited

Loading