Skip to content

Adapt ninja config to use react ppx from upstream syntax submodule #4660

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

Merged
merged 5 commits into from
Oct 25, 2020
Merged

Adapt ninja config to use react ppx from upstream syntax submodule #4660

merged 5 commits into from
Oct 25, 2020

Conversation

jchavarri
Copy link
Contributor

@jchavarri jchavarri commented Aug 31, 2020

Part 2 after rescript-lang/syntax#124 was merged.

Syncs with latest syntax commit.

@jchavarri jchavarri marked this pull request as ready for review October 23, 2020 13:32
@jchavarri
Copy link
Contributor Author

cc @rickyvetter @IwanKaramazow

@jchavarri
Copy link
Contributor Author

There's something in the test that is failing. It seems there's some part that tries to build js_refmt_compiler for testing (?) but it can't find the module Reactjs_jsx_ppx_v3 that is now part of syntax:

> [email protected] test /home/circleci/git/bucklescript
> node scripts/ciTest.js  -all

/home/circleci/git/bucklescript/ocaml/VERSION is used in version detection
File "j.ml", line 323, characters 2-26:
Warning 30: the label comment is defined in both types expression and statement.
File "j.ml", line 337, characters 2-27:
Warning 30: the label comment is defined in both types expression and case_clause.
File "ppx_entry.ml", line 32, characters 11-47:
Error: Unbound module Reactjs_jsx_ppx_v3

I guess somewhere in the linking process napkin.cmxa has to be added, but I have no idea where.

@bobzhang
Copy link
Member

bobzhang commented Oct 24, 2020

if you look at the diff of js_refmt_compiler.ml and js_compiler.ml, the React_..ppx was removed,
I think you need add include paths -I napkin here: https://github.com/rescript-lang/rescript-compiler/blob/master/jscomp/snapshot.ninja#L54 and here https://github.com/rescript-lang/rescript-compiler/blob/master/jscomp/snapshot.ninja#L50

.gitmodules Outdated
@@ -8,5 +8,5 @@
branch = master
[submodule "ninja"]
path = ninja
url = git@github.com:rescript-lang/ninja.git
url = https://github.com/rescript-lang/ninja
Copy link
Member

Choose a reason for hiding this comment

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

do you have issue with git protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't. But I didn't have ssh key configured in the terminal I was using, and updating this submodule failed.

I thought using https would remove one barrier for contributors and I saw other submodules were using it, but can roll this back if needed.

Choose a reason for hiding this comment

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

Let's maybe keep it as it was, to minimize the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jchavarri
Copy link
Contributor Author

Thanks @bobzhang, that seemed to fix it. @IwanKaramazow had already suggested a similar change, but I wrongly thought because there was -I syntax already, it would bring the ppx into scope, but it does not?

@jchavarri jchavarri requested a review from bobzhang October 24, 2020 07:53
@bobzhang
Copy link
Member

@jchavarri the jscomp/syntax directory is for some built-in transformations

@bobzhang bobzhang merged commit f9bafe4 into rescript-lang:master Oct 25, 2020
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