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

Bump serialport to 7.1.5 for Node.js 12 compability #486

Closed
wants to merge 3 commits into from
Closed

Bump serialport to 7.1.5 for Node.js 12 compability #486

wants to merge 3 commits into from

Conversation

nikeee
Copy link
Contributor

@nikeee nikeee commented May 13, 2019

Currently, I'm not able to install the current particle-cli due to compilation errors of node-serialport.

node-serialport fixed Node.js 12 compability in version 7.1.5, see:
https://github.com/serialport/node-serialport/compare/serialport@7.1.4...serialport@7.1.5

@nikeee
Copy link
Contributor Author

nikeee commented May 20, 2019

Anything that prevents this from merging?

@monkbroc
Copy link
Member

Thanks for the contribution. The person who's been working on the Particle CLI the most recently is out of the office at the moment. He'll respond when he's back :)

@busticated
Copy link
Contributor

thanks for the submission @nikeee 🙏👍

Anything that prevents this from merging?

did you notice CI is failing across the board? are you able to see this page?

https://travis-ci.org/particle-iot/particle-cli/builds/531886365

looks like a single unit test and 33 acceptance tests are failing. if you want to try poking at those, it'd be super-helpful. otherwise, i'll add it to my list here.

in the meantime, we recommend you use the stand alone CLI for precisely this reason. installation details here:

https://docs.particle.io/tutorials/developer-tools/cli/#installing

@nikeee
Copy link
Contributor Author

nikeee commented May 23, 2019

I've seen these failing and actually can't tell you why a patch update for node-serialport changes the help text of the CLI. For example this:

Preprocess a Wiring file (ino) into a C++ file (cpp)
Usage: particle preprocess [options] <file>

Options:
  --name    Filename and path to include in the preprocessed file. Default to
            the input file name                                         [string]
  --saveTo  Filename for the preprocessed file                          [string]
  
  Examples:
  particle preprocess app.ino               Preprocess app.ino and save it to
                                            app.cpp
  particle preprocess - --name app.ino      Preprocess from standard input and
  --saveTo -                                save output to standard output.
                                            Useful for scripts

...is checked to contain the string particle preprocess - --name app.ino --saveTo -. Well, it does, but the lines got wrapped, so the test failed. I have no idea why this happens in this PR, since the updates of node-serialport do not contain anything related to that.

Also, some tests fail due to a missing access token to the Particle Cloud:

error: 'invalid_request',\n     error_description: 'The access token was not found' 

Maybe it is a secret that is configured in Travis and won't be available for Pull Requests of external users? There was this security issue some time ago:
https://github.com/ChALkeR/notes/blob/master/Stealing-Travis-secure-variables.md

This PR of an external contributor also contained these errors: #471 The merge commit in master had a successful build.

So I'd suggest that we re-target this PR to a different branch of this project and merge them there. Then we should see which tests really fail.

@busticated
Copy link
Contributor

well, thanks for trying 👍

@nikeee
Copy link
Contributor Author

nikeee commented May 28, 2019

Maybe you could run the tests offline on your machine. I'm certainly missing the authentication tokens for that.

@nikeee
Copy link
Contributor Author

nikeee commented Jul 25, 2019

Any news on this?

@busticated
Copy link
Contributor

no updates as of yet i'm afraid. still on my list though 👍

@nikeee
Copy link
Contributor Author

nikeee commented Aug 13, 2019

This fix was also included in #509, closing.

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

Successfully merging this pull request may close these issues.

3 participants