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

Semver binary version. #101

Closed
wants to merge 1 commit into from
Closed

Semver binary version. #101

wants to merge 1 commit into from

Conversation

lunemec
Copy link

@lunemec lunemec commented Jan 31, 2017

Semver binary.

A first draft.

Semver binary.
@lunemec lunemec mentioned this pull request Jan 31, 2017
@lunemec
Copy link
Author

lunemec commented Jan 31, 2017

I found 1 tiny bit problem. This binary (even musl statically linked) doesn't work inside Alpine linux (docker builder) because of incorrect ELF signature and it wanting to use libc runtime. There is some work to enable rustc to compile it with musl, but not complete yet.

@steveklabnik
Copy link
Contributor

Thanks @lunemec ! I might take a day or two to get to this; this is a release week for Rust, so I'm quite busy. We'll see though.

Copy link
Contributor

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cool! I am worried about dependency count though here. Hrm.

@@ -13,6 +13,13 @@ Semantic version parsing and comparison.

[dependencies]
semver-parser = "0.6.1"
regex = "0.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, one unfortunate aspect of this is that we just removed the regex dependency because it's too heavy. maybe we can not use it here? let' see from below...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking maybe split the binary into separate project, just for the binary. This way you don't pull the dependencies with the library. (Although you'll get them with semver-parser anyways :))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this may or may not be worth it. i'll give it some thought

lazy_static = "0.2.1"

[dependencies.clap]
version = "~2.20.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, you should drop the ~; ^ is the default and is most appropriate, imho.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok :)

}
}

// Borrowed from semver-parser (common is not public).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so this is where regexes get used. I wonder if we can maybe pull this stuff out into an internal crate that both share, so we can not use the regex.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, making common public would help, I was planning to make a PR on that, after you took a look at this (I'm not that proficient in rust). Also, since I'm using regex here, but it is used in the chain - semver -> semver-parser -> regex, does it make a difference for the user? They have to get it anyways...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

semver parser no longer uses regex; I cut a new release without it :)

@lunemec
Copy link
Author

lunemec commented Feb 7, 2017

Ok, I'll cancel this PR, try to rewrite using semver-parser (the new one without regexes). I think I'll send a PR making the method I need public in semver-parser and use that.

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.

2 participants