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

merge reason-parser into main source code #1330

Merged
merged 9 commits into from
Jul 14, 2017
Merged

merge reason-parser into main source code #1330

merged 9 commits into from
Jul 14, 2017

Conversation

let-def
Copy link
Contributor

@let-def let-def commented Jul 6, 2017

As requested in issue #1328.

I kept https://github.com/facebook/reason/compare/master...let-def:collapse-reason-parser?expand=1#diff-8d847b939af98498df86c09cf017a952L24 check but I don't think there is any other subpackage, maybe the check can be removed too.

@@ -93,8 +92,6 @@ You can then run:
-I "$reasonTargetDir" \
-I "$reasonTargetDir/_build/src" \
-I "$reasonTargetDir/vendor/cmdliner" \
-I "$reasonParserTargetDir/_build/src" \
-I "$reasonParserTargetDir/vendor/easy_format" \
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be added back as "$reasonTargetDir/vendor/easy_format", otherwise it's not included.

@@ -21,8 +21,7 @@ if [ "${MASTER}" != "${HEAD}" ]; then
the release. **"
fi

read -p "STOP! Have you made sure to release the sub packages (including \
reason-parser)? (y/n) " yn
read -p "STOP! Have you made sure to release the sub packages? (y/n) " yn
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it's fine to remove it

@chenglou
Copy link
Member

chenglou commented Jul 7, 2017

I can't repro that menhirlib error in the CI. After fixing that reason_bspack.sh line the bspacking workflow works (assuming there's an omp folder with the artifacts of migrate-parsetree, I got it from reason-cli's cache). Also I've merged your #1329 first so there's the rebase conflict (sorry).

@let-def
Copy link
Contributor Author

let-def commented Jul 7, 2017

I tried to fix stuff, but it is still failing and I just can't see the message (seems the CI is temporarily broken too :P).

@chenglou
Copy link
Member

chenglou commented Jul 7, 2017

Ok that's weird. Can you fix the merge conflict and push once and see if the CI retriggers and succeed?

@chenglou
Copy link
Member

chenglou commented Jul 8, 2017

Well, at least it's a new error now

@let-def
Copy link
Contributor Author

let-def commented Jul 8, 2017

I suspect the bug is just the regression with latest utop (which hides some modules, see ocaml-community/utop#213 ).
I will update rtop to use utop-full.

@let-def
Copy link
Contributor Author

let-def commented Jul 8, 2017

Two bugs: the regression + wrong linking rules.
I hope it will work with these :) (I wonder why it didn't fail before in my local builds...)

@chenglou
Copy link
Member

chenglou commented Jul 9, 2017

Btw we're thinking of pulling out rtop into its own thing. Would that help here?

@chenglou
Copy link
Member

@let-def any blockers remaining? Eager to get this in!

@let-def
Copy link
Contributor Author

let-def commented Jul 14, 2017

No I would like to get this in, I am trying to understand what is failing.

@let-def
Copy link
Contributor Author

let-def commented Jul 14, 2017

Seems I just forgot to add a file to the repo, hopefully rebuild works this time!

@chenglou
Copy link
Member

Alright, I've patched it with some last fixes for bspack (which we should really automate and put on CI soon) and verified locally that the bspacking workflow works. Guess we're ready for merging? =D

@let-def
Copy link
Contributor Author

let-def commented Jul 14, 2017

Fine for me :).

@chenglou chenglou merged commit cc9f023 into reasonml:master Jul 14, 2017
@chenglou
Copy link
Member

Thanks so much =)

cc @IwanKaramazow happy now? =P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants