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

Should libuv upgrades be semver-minor? #443

Closed
MylesBorins opened this issue Jan 29, 2019 · 21 comments
Closed

Should libuv upgrades be semver-minor? #443

MylesBorins opened this issue Jan 29, 2019 · 21 comments

Comments

@MylesBorins
Copy link
Contributor

Refs: nodejs/node#24332

I've added the label periodically here is an example of an earlier version marked semver-minor

nodejs/node#18260

1.24.1 and 1.25.0 were both landed as patches as well

nodejs/node#25571
nodejs/node#25078

In general we have not landed these in semver-patch LTS releases, as such I think it would make sense to make it explicit that they are always semver-minor

thoughts?

/cc @cjihrig

@Trott
Copy link
Member

Trott commented Jan 29, 2019

¯\(ツ)

@richardlau
Copy link
Member

libuv follows semver, so I would expect the semverness of the upgrade to be related to the version being upgraded from/to.

@MylesBorins
Copy link
Contributor Author

@richardlau on one hand that seems like a reasonable alternative... on the other I think it is worth considering that a semver-patch change to libuv can be semver-major to Node.js if it breaks behavior

@Trott
Copy link
Member

Trott commented Jan 29, 2019

To clarify my ¯\(ツ)/¯:

On the one hand, semver-minor is for new features and upgrading a dependency without adding a new feature is, uh, by definition not adding a new feature. So: patch!

On the other hand: There's no reason we can't decide to to make it semver-minor out of some abundance of caution?

Yet on the other other hand: What does that get us, exactly? Either way, semver-patch or semver-minor are not supposed to have breaking changes.

Yet back on some other hand somewhere: Psychologically, though, perhaps people think of "minor" bumps as a bigger change than patch, even if that's not strictly semver.

I guess if I had to make a decision: patch unless it introduces a new feature usable by an end user (which would include, say, someone compiling a native addon or whatever).

@Trott
Copy link
Member

Trott commented Jan 29, 2019

a semver-patch change to libuv can be semver-major to Node.js if it breaks behavior

A semver-patch change, by definition, should not do that.

If it does, it's because of a bug. But the same can be said for every semver-patch update.

Although libuv ones are bigger than others...

But that's why we have tests...

So...

¯\(ツ)

@MylesBorins
Copy link
Contributor Author

From semver

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards compatible manner, and
PATCH version when you make backwards compatible bug fixes.

I guess the question comes down to what our definition of "adding functionality is".

As we expose the libuv api (AFAIK) it would seem to make sense to a semver-minor bump to libuv is in turn a semver-minor bump in node.

@Trott
Copy link
Member

Trott commented Jan 29, 2019

A semver-patch change, by definition, should not do that.

Responding to myself because it's that kind of evening: Ah, but then there's the ol' "one person's bugfix is another person's breaking behavior" problem.

But again, semver-minor should not include breaking behavior any more than semver-patch should...

¯\(ツ)

@Trott
Copy link
Member

Trott commented Jan 29, 2019

As we expose the libuv api (AFAIK)

Could you elaborate on that?

@MylesBorins
Copy link
Contributor Author

Could you elaborate on that?

Can people not use the libuv api when writing native modules?

https://nodejs.org/api/addons.html#addons_linking_to_node_js_own_dependencies

Also worth considering that we can have an LTS policy around libuv that is completely unrelated to how they land on master / current

@richardlau
Copy link
Member

richardlau commented Jan 29, 2019

As we expose the libuv api (AFAIK) it would seem to make sense to a semver-minor bump to libuv is in turn a semver-minor bump in node.

I'd have no argument there.

As we expose the libuv api (AFAIK)

Could you elaborate on that?

We statically link libuv into the Node.js binary and provide the headers to addons in our headers tarball (as we do for V8 and OpenSSL and zlib).

@Trott
Copy link
Member

Trott commented Jan 29, 2019

@MylesBorins wrote:

Can people not use the libuv api when writing native modules?

https://nodejs.org/api/addons.html#addons_linking_to_node_js_own_dependencies

@richardlau wrote:

We statically link libuv into the Node.js binary and provide the headers to addons in our headers tarball (as we do for V8 and OpenSSL).

So that would suggest that the semver-ness of the upgrade in Node.js is the same as the semver-ness of the version bump in the dependency itself. And it also suggests that this is not libuv-specific but may also apply to (at least) OpenSSL other dependencies.

Sounds like we're (so far, at least) converging on that....

@richardlau
Copy link
Member

And it also suggests that this is not libuv-specific but also applies to (at least) OpenSSL.

Except OpenSSL doesn't follow semver. (And neither does V8. No idea about zlib, which is the only other dependency we purposely reexport.)

@Fishrock123
Copy link
Contributor

I see no reason why a regular libuv patch release upgrade that does not break node would be semver-minor, if that is what is being asked.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 29, 2019

semver-patch change to libuv can be semver-major to Node.js if it breaks behavior

We build libuv v1.x with Node master and then run the Node test suite before libuv releases now, so that shouldn't be an issue (not to say that things never slip through the cracks).

Since libuv follows semver, and is generally more stable across releases than other dependencies, I see no reason to make all libuv updates semver minor.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2019

@MylesBorins can this be closed?

@MylesBorins
Copy link
Contributor Author

@cjihrig I think we should document the consensus we have in here. I think the release wg meeting on thursday should result in an action item regarding where this should be documented

@BridgeAR
Copy link
Member

@cjihrig
Copy link
Contributor

cjihrig commented May 2, 2019

I'm transferring this to the release repo. The answer here appears to be "no" but the release working group is still discussing it.

@cjihrig cjihrig transferred this issue from nodejs/node May 2, 2019
@mhdawson
Copy link
Member

mhdawson commented May 9, 2019

My thought is that libuv updates should match the semver stated by libuv (ie patch if patch, minor if minor), unless there is a specific reason that warrants something different.

@BridgeAR
Copy link
Member

I added it to the release agenda to discuss this in the next meeting.

I personally do not have a strong opinion but it's probably a good idea to idea to follow the libraries own semver version on our side to include it, if applicable. This can't be applied to dependencies that do not use semver and there might be exceptions where it makes sense to include an update in e.g., a patch release for security reasons or similar even though it's maybe semver-major for the dependency.

@BethGriggs
Copy link
Member

The consensus from #509 was that we will follow the semverness of libuv. If there's a patch upgrade of libuv, this can be included in a patch release of Node.js.

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

No branches or pull requests

8 participants