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

Make scripts work on Windows #94

Closed
StoneCypher opened this issue Apr 21, 2021 · 12 comments
Closed

Make scripts work on Windows #94

StoneCypher opened this issue Apr 21, 2021 · 12 comments

Comments

@StoneCypher
Copy link
Contributor

https://github.com/peggyjs/peggy/blob/main/package.json#L25

"test:node": "nve 10,12,14,15 npm run test:bare",

I would like to advocate that we consider testing node versions in CI, rather than locally, as many of us won't be able to install node version managers, and as that way one doesn't have to wait for four versions of node to go by to test. Also, CI can run them in parallel, and run the entire matrix, and we can exclude versions if we need to (which we ought to)

I can't currently run tests

@hildjj
Copy link
Contributor

hildjj commented Apr 21, 2021

Nod. I use this as a pre-test before pushing. CI is doing 10,12,14,15, soon to move to 12,14,16,17.

Probably nobody but me should use this. That said, nve is cool, and supports the --parallel flag which I should have used here.

I wonder if there is a better way for me to store my personal scripts?

@StoneCypher
Copy link
Contributor Author

It's probably fine to have it as a side script, it just shouldn't be in the main build or main test path, because then it's a practical dependency

Like if this was just named test:node_extra and there was a test:node that used the default version, and nve was made an optional dependency, that'd probably be it, finished

I might advocate that you consider the likelihood that regression behavior in older versions is sufficiently unlikely that, given that the CI rig will be publishing numbered builds, it can't get through either way, and you could be reducing your labor to waiting on the GH interface to say "passed"

I mean if you want this, go ahead, it's not hurting anybody once it's optional and out of the main path

But I suspect you're spending more time than you need to

@hildjj
Copy link
Contributor

hildjj commented Apr 22, 2021

Nod. Once we've got the rollup path in, I probably won't use this anymore. Next time I'm in there, i'll remove it.

@StoneCypher
Copy link
Contributor Author

I'll be doing that this weekend, sorry for the delay

@Mingun
Copy link
Member

Mingun commented Apr 22, 2021

@StoneCypher , are you using Windows or Linux? Because on the Windows bin/peggy from npm run parser task don't run :( :

> npm test

> peggy@1.0.0 pretest
> npm run parser


> peggy@1.0.0 parser
> bin/peggy -o lib/parser.js --format commonjs src/parser.pegjs

"bin" не является внутренней или внешней
командой, исполняемой программой или пакетным файлом.

I'm will be glad to you if you take into account this problem...

@hildjj
Copy link
Contributor

hildjj commented Apr 22, 2021

I never work on this stuff on Windows, so if you want to send a small patch to run peggy correctly, I'll take it. Actually, that's pretty important to me, because I'd like our project to be an example for folks who want to use Peggy in their own projects.

@hildjj
Copy link
Contributor

hildjj commented Apr 22, 2021

I removed the nve references in #89, but I'm going to leave this open with the new title "Make scripts work on Windows"

@hildjj hildjj changed the title Version testing and CI Make scripts work on Windows Apr 22, 2021
@StoneCypher
Copy link
Contributor Author

So I made the scripts work on windows, but that in turn shows that the browserify setup isn't portable

Would you mind if I fix this and rollup at the same time? That's pretty gross, I admit, but it'd help given the wedge

@StoneCypher
Copy link
Contributor Author

image

@StoneCypher
Copy link
Contributor Author

or actually

i could just treat them as separate bugs, that's silly

nevermind

@hildjj
Copy link
Contributor

hildjj commented Apr 22, 2021

Once you make the rollup change, anything browserify should be moot, I hope.

@StoneCypher
Copy link
Contributor Author

should be

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 a pull request may close this issue.

3 participants