search

rust-mobile / cargo-subcommand

Library for implementing cargo subcommands
6 stars 6 forks source link

utils: Strip UNC from canonicalized path which is not supported by glob #6

Closed MarijnS95 closed 3 years ago

MarijnS95 commented 3 years ago

We recently taught fn member() to look for Cargo.toml files in workspace members (globs) relative to the workspace root (the directory containing Cargo.toml with the [workspace] listing of these members). This means the path to the workspace root is now concatenated to the glob path before searching, which didn't happen before. That path happens to be canonicalized just at the top of fn find_package() consequently the whole thing falls apart when Windows UNC paths are used in places they aren't supported, like glob. No paths are returned, the package isn't found (Error::ManifestNotFound below) and cargo-apk is once again broken on Windows.

We have already been discussing to add dunce, now is the right time to get rid of UNC paths in cargo-submodule once and for all.

Fixes: 60fdf8b ("utils: Search for workspace members relative to root manifest (#3)")

CC @dvc94ch @msiglreith

MarijnS95 commented 3 years ago

What was the "Windows UNC paths break cargo-apk" counter at again, 5? So it's at 6 now.

All joking aside, anyone able to help cross-testing this on Windows and Linux just to be absolutely sure I haven't missed a usecase locally?

Guess it's about time to add some CI to cargo-subcommand as well, perhaps just clone android-ndk-rs and try to build it.

dvc94ch commented 3 years ago

Would you like to add some ci? I'll invite you as a collaborator, but I need your email to give you publishing rights to crates.io

MarijnS95 commented 3 years ago

@dvc94ch Would be nice to have yes, saves all the back-and-forth (3 PRs thus far) for simple issues. I'll see if I can think about a way to set up CI here, but that might take a while.

You should be able to extract my email from the commits here :)

dvc94ch commented 3 years ago

mmh that doesn't seem to work either. have you published a crate to crates.io before?

MarijnS95 commented 3 years ago

Err, wait, maybe I should have linked my account first... Because the answer to that question is no. Linked, should be good now!

MarijnS95 commented 3 years ago

Mmmh, normal/rebase merges are not explicitly enabled on this repo... Oops, that tiny arrow is too hard to forget about, sorry @dvc94ch! I can't force-push a merge-less commit either, nor can I adjust that in the settings :grimacing:

dvc94ch commented 3 years ago

You can squash-merge using the triangle icon next to the merge button

MarijnS95 commented 3 years ago

@dvc94ch Yup I'm aware, but it's too tempting to just click the button! Can you disable the other two in https://github.com/dvc94ch/cargo-subcommand/settings just in case?

Also, do you want to correct the history? :P

dvc94ch commented 3 years ago

Nope it's fine, I disabled merge commits, sometimes I like to make a rebase merge on large changes with a clean history

MarijnS95 commented 3 years ago

@dvc94ch Just disabling merge commits is totally fine! You may have noticed that I intentionally separate out topics over multiple commits (each with their own commit message), those fit rebase merges better than squash merges (loosing all the info) anyhow :+1:

MarijnS95 commented 3 years ago

Just performed a release for 0.4.10 including this Windows fix.

@dvc94ch Shall we add tags to the git repo tracking the Release 0.4.x commits?

dvc94ch commented 3 years ago

Knock yourself out. Tagging old releases seems like a waste of time...

MarijnS95 commented 3 years ago

Just the last few should be fine :)