search

paralleldrive / cuid

Collision-resistant ids optimized for horizontal scaling and performance.
Other
3.42k stars 123 forks source link

Fingerprint calculation is updated to avoid exceptions on Windows 7 #264

Open FlyingDR opened 2 years ago

FlyingDR commented 2 years ago

Fixes #263

ericelliott commented 2 years ago

For these reasons, I'm hesitant to merge the proposed change as-is.

FlyingDR commented 2 years ago

Thank you for your comments, I completely agree with you. The only remark is that Microsoft actually provides updates for Windows 7 as a paid option until January 2023.

The main purpose of this change is to allow developers, who work on Windows 7 to be able to run code on their machines for development purposes. Example of such software is Bugsng which relies on the fork of your library: https://github.com/bugsnag/bugsnag-js/issues/1783

I dig deeper into the topic and found a better way that will not compromise collision prevention capabilities. Here is the official documentation from Microsoft for reference.

There is still a fallback to the hardcoded constant for the case if there is some non-Windows host where os.hostname() is also broken. I'm not aware of such situations, so it is just a way to make sure that the hostname will always be a string.

Please take a look at the updated version.

ericelliott commented 2 years ago

Thank you. I like this fix much better.

To merge this, I'm going to want a report from somebody who is not you who has tested this fix. I don't have a Windows 7 machine and don't have time to provision one for testing.

ericelliott commented 2 years ago

Just to be clear - even if we get cuid working with Windows 7 - every other library that relies on the Node hostname would also need fixing. This seems like a bigger effort than EOLing Windows 7, which has already not been getting ANY Node patches for years now... 😄

Perhaps it SHOULD be painful to continue to run Windows 7 with modern software? Is there a really good reason you can't just run an older version of Node?

FlyingDR commented 2 years ago

To merge this, I'm going to want a report from somebody who is not you who has tested this fix. I don't have a Windows 7 machine and don't have time to provision one for testing.

@jeremyspiegel You're an author of https://github.com/bugsnag/bugsnag-js/issues/1783 which is directly related to this issue. Could you please confirm that proposed fix works for you?

FlyingDR commented 2 years ago

Just to be clear - even if we get cuid working with Windows 7 - every other library that relies on the Node hostname would also need fixing. This seems like a bigger effort than EOLing Windows 7, which has already not been getting ANY Node patches for years now... 😄

Of course, Windows 7 is EOL, but its primary purpose nowadays is not to run production code but to allow developers to work on their projects. After all, it is a desktop OS, not Windows Server.

Perhaps it SHOULD be painful to continue to run Windows 7 with modern software? Is there a really good reason you can't just run an older version of Node?

After https://github.com/nodejs/node/issues/33034 enables back modern versions of Node.js back on Windows 7 - this is the first case (besides packages that are not running on Windows at all) when I see the quite popular package to break on this OS. At the same time Node.js itself evolves rapidly and new lots of projects are using language features that are not available in older versions of Node.js. So it is much more painful to stay on older Node.js than to run it on Windows 7 :-)

As I've mentioned already - the actual case (at least for me) is to let Bugsnag integration to work because at this moment their code breaks project compilation on Windows 7. Of course, they're maintaining their own fork https://github.com/bugsnag/cuid, but I think that since cuid is quite popular - there may be other projects that may win from this change to be merged.

Hope that this information will make my intention clearer.

jeremyspiegel commented 2 years ago

The proposed fix works for me: image

FlyingDR commented 2 years ago

@jeremyspiegel Thank you very much for the confirmation!

ericelliott commented 2 years ago

Please update .travis.yml

ericelliott commented 2 years ago

this is the first case (besides packages that are not running on Windows at all) when I see the quite popular package to break on this OS

I guarantee, cuid is not the only library in the Node ecosystem that depends on os.hostname - in fact this whole thread is about people being upset about the change in libuv.

Of course, Windows 7 is EOL, but its primary purpose nowadays is not to run production code but to allow developers to work on their projects. After all, it is a desktop OS, not Windows Server.

There are lots of other desktop operating systems those people could switch to, and probably should because it's unsafe to run Windows 7 as a desktop OS due to the fact that Microsoft is no longer supporting it. Bugs are not being patched. Security software for Windows 7 is not getting updated. A lot of modern software just doesn't run at all on Windows 7 because they depend on newer APIs that didn't exist when Windows 7 was actively maintained.

As I've mentioned already - the actual case (at least for me) is to let Bugsnag integration to work because at this moment their code breaks project compilation on Windows 7. Of course, they're maintaining their own fork https://github.com/bugsnag/cuid, ...

If they're maintaining their own fork, a fix here won't fix your problem anyway. You'll need to open a PR in their fork.

but I think that since cuid is quite popular - there may be other projects that may win from this change to be merged.

So far, this is the only ticket that has been opened to fix cuid on Windows 7, and I haven't seen any upvotes on it, which tells me the demand for this fix is very low. Typically, if CUID breaks a build, we get LOTS of people complaining very quickly. 🤣

This is why I'm being so weird about this patch (or any patch to CUID). If we break a build, we break big chunks of the internet.

FlyingDR commented 2 years ago

Please update .travis.yml

Isn't updating the .travis.yml out of the scope of this pull request? May it be better to have a separate pull request for this purpose and rebase this one? Please suggest.

I understand your position and of course, it is up to you to merge this pull request. I will also propose the Bugsnag team fix their fork, planned to do it after the merge in the upstream, but the local fix is also acceptable of course.

ericelliott commented 2 years ago

No, it is not out of scope.

FlyingDR commented 2 years ago

I've updated Travis CI configuration to use the minimal currently supported version of Ubuntu and currently supported versions of Node.js.

The setup of Chrome is updated to the currently supported way.

FlyingDR commented 2 years ago

@ericelliott It looks like it is required to make a change in the repository configuration from your side.

Travis CI is migrated from travis-ci.org to the travis-ci.com (docs, blog posts: 1, 2).

It is not working for more than a year on the travis-ci.org (notice a banner and a date of the last build) and on travis-ci.com there are no builds, at least publicly visible ones.

Since the last change for the Travis CI configuration was made more than 4 years ago - the current integration configuration is probably needed to be updated and I can't do anything about it. Please review and update from your side.

ericelliott commented 1 year ago

I got the repo configured to run Travis builds again. I don't have time to keep debugging this though, and this is a very low priority for me because Windows 7 was end of life 2 years ago. If you want this merged, it's up to you to push this forward.

You can see the new Travis build status at https://app.travis-ci.com/github/paralleldrive/cuid

I can't add the Travis build requirement to the branch protection rules, yet, so you may need to check it manually.