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

Get rid of Makefile.nt in the yacc directory. #762

Merged
merged 1 commit into from Aug 26, 2016

Conversation

Projects
None yet
3 participants
@shindere
Contributor

shindere commented Aug 12, 2016

The Makefile has been changed so that it also works on Windows.

As a consequence, in OCaml's main Makefile.nt, building things in the
yacc directory can now be done by invoking $(MAKE) rather than
$(MAKEREC).

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Aug 24, 2016

Contributor

Is this sensible to do piecemeal? I though other instances simply leave Makefile.nt as a simple include once the main Makefile is updated?

Contributor

dra27 commented Aug 24, 2016

Is this sensible to do piecemeal? I though other instances simply leave Makefile.nt as a simple include once the main Makefile is updated?

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 24, 2016

Contributor
Contributor

shindere commented Aug 24, 2016

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 24, 2016

Contributor

I think it's fine to eliminate duplication directory by directory (this is how we have been proceeding for some time). The question from @dra27 (I think) is whether we want to leave a tiny Makefile.nt which only includes Makefile. And when all Makefile.nt have been made trivial this way, we get rid of all of them at once. The benefit of keeping Makefile.nt is that developers under Windows can simply do "make -f Makefile.nt" (as they are used to do) anywhere, instead of having to do either that or just "make" depending on the directory.

Contributor

alainfrisch commented Aug 24, 2016

I think it's fine to eliminate duplication directory by directory (this is how we have been proceeding for some time). The question from @dra27 (I think) is whether we want to leave a tiny Makefile.nt which only includes Makefile. And when all Makefile.nt have been made trivial this way, we get rid of all of them at once. The benefit of keeping Makefile.nt is that developers under Windows can simply do "make -f Makefile.nt" (as they are used to do) anywhere, instead of having to do either that or just "make" depending on the directory.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 24, 2016

Contributor
Contributor

shindere commented Aug 24, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 24, 2016

Contributor
Contributor

shindere commented Aug 24, 2016

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 24, 2016

Contributor

Any idea on this?

Let's ask our git expert. @gasche?

Contributor

alainfrisch commented Aug 24, 2016

Any idea on this?

Let's ask our git expert. @gasche?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 24, 2016

Contributor

Is it okay to replace the calls to $(MAKEREC) by calls to $(MAKE) (in the main Makefile.nt) right now (as is done in the two PRs)

I would have a slight preference for not doing it, in order to ensure that the Makefile.nt won't be removed too early by someone else who finds out that it is not used anymore.

Contributor

alainfrisch commented Aug 24, 2016

Is it okay to replace the calls to $(MAKEREC) by calls to $(MAKE) (in the main Makefile.nt) right now (as is done in the two PRs)

I would have a slight preference for not doing it, in order to ensure that the Makefile.nt won't be removed too early by someone else who finds out that it is not used anymore.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 24, 2016

Contributor

Alain Frisch (2016/08/24 03:01 -0700):

Is it okay to replace the calls to $(MAKEREC) by calls to $(MAKE) (in the main Makefile.nt) right now (as is done in the two PRs)

I would have a slight preference for not doing it, in order to ensure
that the Makefile.nt won't be removed too early by someone else who
finds out that it is not used anymore.

I get your point. I'm still unsure because not doing the replacement now
also means that the fact that the build system works without the
Makefile.nt is not verified. Moreover, I hope to complete the transition
in not too long (typically a few weeks, less than a month probably), so
I'm not sure how high the risk is that somebody accidentally removes a
file or so.

Contributor

shindere commented Aug 24, 2016

Alain Frisch (2016/08/24 03:01 -0700):

Is it okay to replace the calls to $(MAKEREC) by calls to $(MAKE) (in the main Makefile.nt) right now (as is done in the two PRs)

I would have a slight preference for not doing it, in order to ensure
that the Makefile.nt won't be removed too early by someone else who
finds out that it is not used anymore.

I get your point. I'm still unsure because not doing the replacement now
also means that the fact that the build system works without the
Makefile.nt is not verified. Moreover, I hope to complete the transition
in not too long (typically a few weeks, less than a month probably), so
I'm not sure how high the risk is that somebody accidentally removes a
file or so.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Aug 24, 2016

Contributor

Are you presently working on merging the entire build system (total elimination of Makefile.nt)?

Contributor

dra27 commented Aug 24, 2016

Are you presently working on merging the entire build system (total elimination of Makefile.nt)?

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 24, 2016

Contributor

David Allsopp (2016/08/24 04:51 -0700):

Are you presently working on merging the entire build system (total
elimination of Makefile.nt)?

Yes. Unless there is an objection agains it, it seems to me it would be
better to have the two systems merged before starting the work on
autoconf.

Of course, any comment is more than welcome.

Contributor

shindere commented Aug 24, 2016

David Allsopp (2016/08/24 04:51 -0700):

Are you presently working on merging the entire build system (total
elimination of Makefile.nt)?

Yes. Unless there is an objection agains it, it seems to me it would be
better to have the two systems merged before starting the work on
autoconf.

Of course, any comment is more than welcome.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Aug 24, 2016

Contributor

Definitely no objection - that's awesome! It was on my list for post-OPAM, but no way I'd be getting to it before the middle of next year! If you need any help/testing (especially with the FlexDLL bootstrapping stuff), do ping me.

Personally, I'd put all the changes through as one pull request (with lots of commits), but that's just a preference.

Contributor

dra27 commented Aug 24, 2016

Definitely no objection - that's awesome! It was on my list for post-OPAM, but no way I'd be getting to it before the middle of next year! If you need any help/testing (especially with the FlexDLL bootstrapping stuff), do ping me.

Personally, I'd put all the changes through as one pull request (with lots of commits), but that's just a preference.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 24, 2016

Contributor
Contributor

shindere commented Aug 24, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 24, 2016

Contributor
Contributor

shindere commented Aug 24, 2016

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Aug 24, 2016

Contributor

If it'd helpful (it'd certainly be interesting for me!), can we have a chat about the directions for this? It's an area that I've been musing on a lot but, with other pressures, not done a huge amount with, and it'd be good to know where everyone else's thoughts are too!

Contributor

dra27 commented Aug 24, 2016

If it'd helpful (it'd certainly be interesting for me!), can we have a chat about the directions for this? It's an area that I've been musing on a lot but, with other pressures, not done a huge amount with, and it'd be good to know where everyone else's thoughts are too!

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 24, 2016

Contributor
Contributor

shindere commented Aug 24, 2016

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 25, 2016

Contributor

@dra27 Can you comment on whether you'd prefer to keep calling Makefile.nt (instead of just Makefile) from the root Makefile.nt for those subdirectories where the merger is done?

Contributor

alainfrisch commented Aug 25, 2016

@dra27 Can you comment on whether you'd prefer to keep calling Makefile.nt (instead of just Makefile) from the root Makefile.nt for those subdirectories where the merger is done?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Aug 25, 2016

Contributor

Yes, cf. lex/Makefile.nt and otherlibs/{raw_spacetime_lib,dynlink}/Makefile.nt.

Contributor

dra27 commented Aug 25, 2016

Yes, cf. lex/Makefile.nt and otherlibs/{raw_spacetime_lib,dynlink}/Makefile.nt.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 25, 2016

Contributor

not doing the replacement now also means that the fact that the build system works without the Makefile.nt is not verified

Considering that Makefile.nt is just an include of Makefile, this can hardly break. Considering @dra27 preference, I would suggest keeping the call to sub Makefile.nt files from the root one (i.e. use $(MAKEREC)).

Can you apply the change, also to the other similar PR?

Contributor

alainfrisch commented Aug 25, 2016

not doing the replacement now also means that the fact that the build system works without the Makefile.nt is not verified

Considering that Makefile.nt is just an include of Makefile, this can hardly break. Considering @dra27 preference, I would suggest keeping the call to sub Makefile.nt files from the root one (i.e. use $(MAKEREC)).

Can you apply the change, also to the other similar PR?

Merge Makefile.nt into Makefile in the yacc directory.
yacc/Makefile has been changed so that it also works on Windows.
@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 25, 2016

Contributor

Alain Frisch (2016/08/25 00:06 -0700):

Considering that Makefile.nt is just an include of Makefile, this can
hardly break. Considering @dra27 preference, I would suggest keeping
the call to sub Makefile.nt files from the root one (i.e. use
$(MAKEREC)).

Okay.

Can you apply the change, also to the other similar PR?

Sure. Both PRs have now been updated and rebased.

Thanks a lot for your comments @alainfrisch and @dra27!

Contributor

shindere commented Aug 25, 2016

Alain Frisch (2016/08/25 00:06 -0700):

Considering that Makefile.nt is just an include of Makefile, this can
hardly break. Considering @dra27 preference, I would suggest keeping
the call to sub Makefile.nt files from the root one (i.e. use
$(MAKEREC)).

Okay.

Can you apply the change, also to the other similar PR?

Sure. Both PRs have now been updated and rebased.

Thanks a lot for your comments @alainfrisch and @dra27!

@alainfrisch alainfrisch merged commit d917ff6 into ocaml:trunk Aug 26, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

alainfrisch added a commit that referenced this pull request Aug 26, 2016

@shindere shindere deleted the shindere:merge-yacc-makefiles branch Aug 26, 2016

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge Makefile.nt into Makefile in the yacc directory. (#762)
yacc/Makefile has been changed so that it also works on Windows.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

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