Skip to content

Add flag: non-standard-requirements #5402

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

Closed
wants to merge 1 commit into from

Conversation

csmoe
Copy link
Member

@csmoe csmoe commented Apr 22, 2018

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@csmoe csmoe force-pushed the non_standard branch 3 times, most recently from e049dbf to daf1ebd Compare April 22, 2018 09:00
@matklad
Copy link
Member

matklad commented Apr 22, 2018

@csmoe this looks great! Would you mind opening a PR against semver as well? We can only use published crates. I am not actually sure that is_semver_open is necessary the best API extension, @steveklabnik would know better!

@csmoe
Copy link
Member Author

csmoe commented Apr 22, 2018

@matklad Here is the VersionReq API:

    pub fn is_semver_open(&self) -> bool {
        if self.predicates.is_empty() {
            return true;
        }

        self.predicates.iter().all(|p| {
            match p.op {
                Ex | // `=`
                Lt | // `<`
                LtEq | // `<=`
                Gt | // `>`
                GtEq | // `>=`
                Tilde | // `~`
                Wildcard(_) => false, // `*`
                Compatible => true, // deal with "1.0.0" or "^1.0.0"
            }
        })
    }

^x.y.z is not necessary, because ^ is default

That means that any semver requirement, except for ^x.y.z, does not make sense.

These two statements seem to contradict each other. So as the comment writes above, I didn't address the first rule(^ is non-semver-open).

Before PR to semver, should I split Compatible to "" and "^" like(draft):

...
match p.op {
    ...
    Compatible => {
        if is_^(op) {
            return false; // `^` is non-semver-open
        } else {
            return true;
        }
    }
     ...
}
...

@matklad
Copy link
Member

matklad commented Apr 22, 2018

These two statements seem to contradict each other. So as the comment writes above, I didn't address the first rule(^ is non-semver-open).

Yeah, I've written something very confusing :-) I mean that specifying ^ is not necessary, because that is a default.

@matklad
Copy link
Member

matklad commented Apr 22, 2018

Before PR to semver, should I split Compatible to "" and "^" like(draft):

Probably... And I think a better name for "semver-open" should be invented, because ^1.2.0 is semver open, but you shouldn't use it anyway, because 1.2.0 form is preferable. Perhaps fn is_plain_semver()? Also, I think self.predicates.is_empty() should perhaps be replaced with self.predicates.len() == 1?

@csmoe
Copy link
Member Author

csmoe commented Apr 22, 2018

@matklad Thank for your suggestions.

is_plain_semver(): ^1.2.0 is semver open, but you shouldn't use it anyway, because 1.2.0 form is preferable.

So is_plain_semver is stricter than is_open_semver since it exclude any version manifest except 1.0.0. I'll address your mentoring instructions later.

I find that "1.0.0" and "^1.0.0" were simply marked Compatible as whole, it seems no way to split them. So a PR to semver-parser is needed:

enum CompatibleOp {
    Caret, // `^1.2.0`
    Default_, // `1.2.0`
}

More commits tomorrow.

@csmoe
Copy link
Member Author

csmoe commented Apr 23, 2018

@matklad
ohh, that's more complicated than I thought since semver currently depends on semver-parser v0.7.0, but the latest semver-parser codebase has changed a lot(real breaking changes).
So PR to semver will be blocked before those 2 PRs done:

  1. rebase CompitableOp(currently based on v0.7.0) onto semver-parser lastest.
  2. merge semver-parser lastest into semver.

@matklad
Copy link
Member

matklad commented Apr 23, 2018

@csmoe oh, I didn’t know about semver-parser at all! I think this actually makes things easier: we can use semver-parser directly. It exposes the internal structure of versions, so we don’t need any API changes and can implement all checks in Cargo!

@alexcrichton
Copy link
Member

@matklad FWIW while I do agree we should tackle something like #5340 long-term I don't think we should start warning today, even for the epoch. The last discussion around this for publishing to crates.io (which ended up only disallowing *) IIRC had a lot of discussion and I think that if we wanted to institute a change like this we'd likely want to reboot some of that discussion and otherwise more broadly advertise this change. I think it's fine to land this functionality gated for now, but I'd prefer to not enable by default just yet

@bors
Copy link
Contributor

bors commented May 3, 2018

☔ The latest upstream changes (presumably #5473) made this pull request unmergeable. Please resolve the merge conflicts.

@matklad
Copy link
Member

matklad commented May 29, 2018

@csmoe sorry for letting this to fall through the cracks!

The two things to do here I think 1) switch to semver parser from semver 2) feature-gate this functionality.

For 2, you'll need to add a flag to CliUnstable:

/// A parsed representation of all unstable flags that Cargo accepts.
///
/// Cargo, like `rustc`, accepts a suite of `-Z` flags which are intended for
/// gating unstable functionality to Cargo. These flags are only available on
/// the nightly channel of Cargo.
///
/// This struct doesn't have quite the same convenience macro that the features
/// have above, but the procedure should still be relatively stable for adding a
/// new unstable flag:
///
/// 1. First, add a field to this `CliUnstable` structure. All flags are allowed
/// to have a value as the `-Z` flags are either of the form `-Z foo` or
/// `-Z foo=bar`, and it's up to you how to parse `bar`.
///
/// 2. Add an arm to the match statement in `CliUnstable::add` below to match on
/// your new flag. The key (`k`) is what you're matching on and the value is
/// in `v`.
///
/// 3. (optional) Add a new parsing function to parse your datatype. As of now
/// there's an example for `bool`, but more can be added!
///
/// 4. In Cargo use `config.cli_unstable()` to get a reference to this structure
/// and then test for your flag or your value and act accordingly.
///
/// If you have any trouble with this, please let us know!
#[derive(Default, Debug)]
pub struct CliUnstable {
pub print_im_a_teapot: bool,
pub unstable_options: bool,
pub offline: bool,
pub no_index_update: bool,
pub avoid_dev_deps: bool,
pub minimal_versions: bool,
pub package_features: bool,
}

@csmoe
Copy link
Member Author

csmoe commented May 29, 2018

@matklad relist this into my TODO.

FWIW while I do agree we should tackle something like #5340 long-term I don't think we should start warning today, even for the epoch.

@alexcrichton
Copy link
Member

@csmoe any luck figuring out the test failures and moving this to a gated feature?

@alexcrichton
Copy link
Member

Ok I'm gonna close this due to inactivity, but feel fre of course to resubmit!

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.

5 participants