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

Fix the preprocessing actions #128

Merged
merged 1 commit into from Feb 7, 2019
Merged

Fix the preprocessing actions #128

merged 1 commit into from Feb 7, 2019

Conversation

ghost
Copy link

@ghost ghost commented Feb 6, 2019

Preprocessing actions shouldn't manually redirect the output to ${@} as dune already does this. This used to work, however it no longer works with dune >= 1.6.3 (see ocaml/dune#1811 for details).

We plan to make this a hard error in dune, to avoid other cases in the future.

Preprocessing actions shouldn't manually redirect the output to ${@}
as dune already does this. This used to work, however it no longer
works with dune >= 1.6.3 due to changes to the code executing actions.

See ocaml/dune#1811 for details.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ghost
Copy link
Author

ghost commented Feb 6, 2019

Do I need to sign a CLA for this patch? It is very small and trivial...

@AltGr
Copy link
Member

AltGr commented Feb 6, 2019

You can ignore the CLA ;)
We should probably update the opam file to specify the minimum version of dune though ? (The dependency is still to jbuilder)

@Drup
Copy link
Collaborator

Drup commented Feb 6, 2019

@AltGr We could just upgrade to dune on the way and ditch jbuilder couldn't we ?

@ghost
Copy link
Author

ghost commented Feb 6, 2019

Before this patch there should be a conflict with dune >= 1.6.3, but after this patch the conflict is no longer needed, i.e. the new code works with both old and new versions of dune. So we should update the opam file in the opam-repository, but it is not necessary to update the one in this repository.

Regarding moving to dune, we now have an automatic upgrader in dune master; running dune upgrade will convert the project to dune (except for the calls to jbuilder in the Makefile).

@AltGr
Copy link
Member

AltGr commented Feb 6, 2019

great, thanks!

@Drup
Copy link
Collaborator

Drup commented Feb 7, 2019

Let's merge, so that we can at least pin master! :)

@Drup Drup merged commit aaebf27 into OCamlPro:master Feb 7, 2019
@UnixJunkie
Copy link

There should be a bugfix release also.
Thanks!

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.

5 participants