Fingerprint calculation is updated to avoid exceptions on Windows 7 #264
Conversation
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: bugsnag/bugsnag-js#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 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? |
@jeremyspiegel You're an author of bugsnag/bugsnag-js#1783 which is directly related to this issue. Could you please confirm that proposed fix works for you? |
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.
After nodejs/node#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 Hope that this information will make my intention clearer. |
|
The proposed fix works for me: |
|
@jeremyspiegel Thank you very much for the confirmation! |
|
Please update |
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.
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.
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.
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. |
Isn't updating the 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. |
|
No, it is not out of scope. |
…d Node.js for tests
|
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 It is not working for more than a year on the 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. |
Fixes #263