cuid
cuid copied to clipboard
ericelliott
Fingerprint calculation is updated to avoid exceptions on Windows 7
- This solution as presented will significantly reduce the collision prevention capabilities of all the affected hosts. In other words, cuid won't do what cuid was intended to do on Windows 7.
- Microsoft's official support for Windows 7 ended January 14, 2020.
- The latest Node.js version that officially supports Windows 7 is 13.6.0 (January, 2020)
- There may be a workaround for people who are stuck in Window 7 pergatory.
- I don't have a windows 7 machine to test this patch on.
- Every change to cuid could potentially break thousands of applications.
- CI/CD is stalled for some reason, which means the automated checks never ran for this PR. 😞 Probably because we need to update
.travis.ymlto run on currently supported versions of Node, and we should probably remove thedist:key entirely.
For these reasons, I'm hesitant to merge the proposed change as-is.
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.
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.
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?
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?
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.
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.
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.
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.
@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.
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.