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

Add js.event, js.json & js.yaml, using JS yaml package #20

Closed
wants to merge 5 commits into from

Conversation

eemeli
Copy link
Member

@eemeli eemeli commented Jul 2, 2018

This adds yaml to the editor. Its JS package is currently installable with npm install yaml, but I've here followed the pattern of other packages and built it from source. It should pass almost all of the applicable test-suite tests, but it's not instrumented (at least yet) for the event format.

As yaml requires at least node v6, I've also updated the config to use node v8, the current LTS version that first came out in 2017.

There's a slight naming conflict that this package introduces, as the other JS package is js-yaml, and I couldn't figure out a better naming pattern than calling mine just js. If you can recommend an alternative, I'd be happy to rename the rules for one or both libraries.

@perlpunk
Copy link
Member

perlpunk commented Jul 4, 2018

Thanks, this looks good at a first glance.
Maybe @ingydotnet can have a closer look at the nodejs stuff.
Also no idea about the naming.
We're very busy currently with pyyaml & libyaml releases, though...

@eemeli eemeli changed the title Add js.json & js.yaml, using JS yaml package Add js.event, js.json & js.yaml, using JS yaml package Jul 9, 2018
@eemeli
Copy link
Member Author

eemeli commented Jul 9, 2018

I added a test harness to also generate an event stream. I also configured my local instance of yaml-test-matrix to use this; the only tests that are reading as errors for me are 2JQS and PW8X, both of which are due to yaml/yaml-test-suite#25. A few of the JSON tests (8G76, 98YD, and M7A3) are registering as different, due to yaml including empty documents in its JSON output to allow for their comments to not be lost.

Anything I can do to help get this merged?

@eemeli
Copy link
Member Author

eemeli commented Jul 13, 2018

@perlpunk @ingydotnet Anything I could do to help move this ahead?

@perlpunk
Copy link
Member

perlpunk commented Aug 4, 2018

We're both pretty busy, preparing for another conference in two weeks...
Hopefully we can have a look then

@perlpunk
Copy link
Member

At the conference I tried out the PR, but got an error:

% make build
...
docker run --rm -v .../YAML:/work yamlio/builder-node node-modules js
+ [[ js =~ js-yaml ]]
+ cd /work/js/
+ npm install .
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.4 (node_modules/fsevents):
up to date in 8.905s
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.4: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

+ npm run build

> yaml@1.0.0-rc.7 build /work/js
> babel src/ --out-dir dist/

Successfully compiled 44 files with Babel.
+ cd /work/build/
+ npm install --no-save /work/js/
npm WARN enoent ENOENT: no such file or directory, open '/work/build/package.json'
npm WARN build No description
npm WARN build No repository field.
npm WARN build No README data
npm WARN build No license field.

+ yaml@1.0.0-rc.7
updated 1 package in 5.799s
+ vcs-info js
+ versiondir=/work/build/versions
+ [[ -d /work/build/versions ]]
+ [[ -d .git ]]
+ [[ -d .hg ]]
+ set-perms
++ uname
+ [[ Linux == Darwin ]]
++ stat -c%u /work/yaml-editor/docker/bin/set-perms
+ uid=1001
++ stat -c%g /work/yaml-editor/docker/bin/set-perms
+ gid=1001
+ chown -R 1001.1001 /work/build
touch build/node_modules/yaml/dist/index.js
touch: cannot touch 'build/node_modules/yaml/dist/index.js': Permission denied
Makefile:135: recipe for target 'build/node_modules/yaml/dist/index.js' failed
make[1]: *** [build/node_modules/yaml/dist/index.js] Error 1
make[1]: Leaving directory '.../YAML/yaml-editor/docker'
Makefile:4: recipe for target 'build' failed
make: *** [build] Error 2

@perlpunk
Copy link
Member

perlpunk commented Aug 26, 2018

I tried copying the source directory to /tmp to avoid changing the original, but getting a different error now.

...
+ npm install --no-save /tmp/js-build/
npm WARN enoent ENOENT: no such file or directory, open '/work/build/package.json'
npm WARN build No description
npm WARN build No repository field.
npm WARN build No README data
npm WARN build No license field.

+ yaml@1.0.0-rc.8
added 1 package in 4.015s
+ vcs-info js
+ versiondir=/work/build/versions
+ [[ -d /work/build/versions ]]
+ [[ -d .git ]]
+ [[ -d .hg ]]
+ set-perms
++ uname
+ [[ Linux == Darwin ]]
++ stat -c%u /work/yaml-editor/docker/bin/set-perms
+ uid=1001
++ stat -c%g /work/yaml-editor/docker/bin/set-perms
+ gid=1001
+ chown -R 1001.1001 /work/build
touch build/node_modules/yaml/dist/index.js
touch: cannot touch 'build/node_modules/yaml/dist/index.js': No such file or directory
Makefile:135: recipe for target 'build/node_modules/yaml/dist/index.js' failed

I pushed my changes to branch PR/20

@ingydotnet would be nice if you could have a look

@eemeli
Copy link
Member Author

eemeli commented Aug 31, 2018

@perlpunk Could it be that the permissions for your docker/bin/set-perms file or your Docker environment are not quite right? I followed the same pattern the other build scripts used, and this error does not seem to relate to node specifically, but to how the permissions are set from within the container, but the touch is run outside the container.

@perlpunk
Copy link
Member

@eemeli I think it's because the build process changes the source directory (adds directories and files).

I actually started to change all the build scripts to first copy the source directory to /tmp, because in another case it resulted in a newer change time for the directory which made make doing the build over and over again.

@eemeli
Copy link
Member Author

eemeli commented Sep 1, 2018

Ah, now I see. I'm pretty sure that the problem is due to the updated npm handling npm install differently than before for local paths; the dependencies now become symlinks rather than copies, which means that their contained files won't be touched by chown -R.

I just pushed a fix that should work around this issue.

@perlpunk
Copy link
Member

perlpunk commented Sep 2, 2018

Good news! I was able to build.
I added my commit to copy the source directory to /tmp anyway, just to avoid issus like that in the future.

I'll run the test-matrix to check if everything is still working (since one of the underlying images were updated, so probably some ubuntu package updates).

I'm still not quite happy with the naming. Maybe we could prefix the js packages with nodejs? So we'd have nodejs-js-yaml.json and nodejs-yaml.{event,json,yaml}. Thoughts? @ingydotnet?

@perlpunk
Copy link
Member

perlpunk commented Sep 2, 2018

For some reason ruamel.yaml is now broken. It adds \a to some block scalars. Have to investigate...

@perlpunk
Copy link
Member

perlpunk commented Sep 8, 2018

ok, the \a thing was apparently a bug in ruamel 0.15.64: https://pypi.org/project/ruamel.yaml/0.15.66/

For some reason our setup isn't installing our cloned hg source of ruamel, but the latest pypi version. Will have to fix that later.

I pulled the ubuntu image and now rebuilding, and hopefully I'll get the fixed version of ruamel.

@perlpunk
Copy link
Member

perlpunk commented Sep 8, 2018

Ok, rebuild looks good, just running the test matrix on your processor, excellent results!

Will merge as soon as possible, maybe not tonight though

@perlpunk
Copy link
Member

sorry, this was a crazy week.
I just merged the PR with my additions, rebuilt and pushed the matrix http://matrix.yaml.io/ and pushed the docker image https://hub.docker.com/r/yamlio/yaml-editor/

Thanks, your project looks very promising and currently has the best results, and I hope I get time to look a bit closer at it soon!

@perlpunk
Copy link
Member

perlpunk commented Oct 4, 2018

Forgot to close ;-)

@perlpunk perlpunk closed this Oct 4, 2018
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.

None yet

2 participants