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

Fix argon2 with special case #53

Merged
merged 4 commits into from Aug 20, 2019
Merged

Fix argon2 with special case #53

merged 4 commits into from Aug 20, 2019

Conversation

styfle
Copy link
Member

@styfle styfle commented Aug 19, 2019

Fixes #45

This package doesn't seem to work on Windows so I excluded from the Windows CI.

@styfle styfle requested a review from lucleray as a code owner August 19, 2019 19:44
@styfle styfle added the automerge Automatically merge PR once checks pass label Aug 20, 2019
Copy link
Member

@lucleray lucleray left a comment

Choose a reason for hiding this comment

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

npm install argon doesn't seem to work on Windows

@styfle
Copy link
Member Author

styfle commented Aug 20, 2019

@lucleray This package doesn't seem to install at all on Windows so I excluded from the Windows CI. The most important test here is for Linux.

@codecov-io
Copy link

Codecov Report

Merging #53 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   89.97%   90.01%   +0.03%     
==========================================
  Files          10       10              
  Lines         988      991       +3     
==========================================
+ Hits          889      892       +3     
  Misses         99       99
Impacted Files Coverage Δ
src/utils/special-cases.js 95.52% <100%> (+0.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73614b5...fa9e83f. Read the comment docs.

@styfle
Copy link
Member Author

styfle commented Aug 20, 2019

@lucleray It turns out that Linux and Mac needed different binary paths so I included both. Windows of course doesn't install at all so its still excluded.

@styfle styfle requested a review from lucleray August 20, 2019 20:17
@kodiakhq kodiakhq bot merged commit ff235b7 into master Aug 20, 2019
@kodiakhq kodiakhq bot deleted the argon branch August 20, 2019 20:27
@styfle styfle mentioned this pull request Aug 20, 2019
@ranisalt
Copy link

ranisalt commented Oct 1, 2019

Hi! I'm the maintainer for Argon2. What do you mean that it does not work on Windows? We have had CI running on Windows for a while with no issues, and while some users report errors on compilation, it's usually due to missing build tools. Could you elaborate so we can fix it?

@styfle
Copy link
Member Author

styfle commented Oct 1, 2019

Hi @ranisalt

The commits above have the CI results from GitHub Actions.

Take a look at this one https://github.com/zeit/node-file-trace/runs/198427474

@ranisalt
Copy link

ranisalt commented Oct 1, 2019

node-gyp output is horribly awful, but from
gyp ERR! stack Error: spawn C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\15.0\Bin\MSBuild.exe ENOENT I think that Actions doesn't have VS build tools installed. You can see from the list of available software, I don't recognize anything there for compiling C++.

@styfle
Copy link
Member Author

styfle commented Oct 2, 2019

That's strange because the linked document says it contains cmake.exe and MinGW 🤷‍♂

Is this something that needs to be reported to the GH Actions team or do we need to install something in CI before building?

@ranisalt
Copy link

ranisalt commented Oct 2, 2019

Honestly I don't know, my builds on Windows are failing too. We have Appveyor working but there you can pick a Visual Studio version and it installs all components, compilers included. Maybe on Windows we can override compiler location with an environment variable, but I have no clue how.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PR once checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for argon2
4 participants