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

Make .depend files generated for C sources more portable #1047

Merged
merged 7 commits into from Mar 9, 2017

Conversation

Projects
None yet
4 participants
@shindere
Contributor

shindere commented Feb 17, 2017

This implements a suggestion provided by @xavierleroy in
#941 (comment)

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 17, 2017

Contributor

Looks good to me - is this is a good time to address the dependencies in win32unix (which appear not to include C files at all?)

Contributor

dra27 commented Feb 17, 2017

Looks good to me - is this is a good time to address the dependencies in win32unix (which appear not to include C files at all?)

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 17, 2017

Contributor
Contributor

shindere commented Feb 17, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 17, 2017

Contributor

Updated to also take into account the dependencies of the C files
of the win32unix library, thanks again for the reminder, @dra27.

Contributor

shindere commented Feb 17, 2017

Updated to also take into account the dependencies of the C files
of the win32unix library, thanks again for the reminder, @dra27.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 17, 2017

Contributor

In favor! (unsurprisingly)

Contributor

xavierleroy commented Feb 17, 2017

In favor! (unsurprisingly)

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 18, 2017

Contributor

Travis fails because of no change entry, so I added the no-change-entry-needed label so that we can make some progress. I'm more worried by the AppVeyor failure, which looks like a real problem. Please check.

Contributor

xavierleroy commented Feb 18, 2017

Travis fails because of no change entry, so I added the no-change-entry-needed label so that we can make some progress. I'm more worried by the AppVeyor failure, which looks like a real problem. Please check.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 20, 2017

Contributor
Contributor

shindere commented Feb 20, 2017

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 20, 2017

Contributor

"Files unixLables.cmo and unix.cmo make inconsistent assumptions over interface Unix". I don't know how to deal with this, either.

This often comes from missing dependencies causing some files not to be remade. It so happens that you're in the process of changing dependencies...

Isn't it justa a matter of really cleaning the build environment and running the build again? Perhaps the appveyor scriptshould make sure to do a make distclean before starting to compile?

No, because in this case we would find fewer 'missing dependencies' issues.

Contributor

xavierleroy commented Feb 20, 2017

"Files unixLables.cmo and unix.cmo make inconsistent assumptions over interface Unix". I don't know how to deal with this, either.

This often comes from missing dependencies causing some files not to be remade. It so happens that you're in the process of changing dependencies...

Isn't it justa a matter of really cleaning the build environment and running the build again? Perhaps the appveyor scriptshould make sure to do a make distclean before starting to compile?

No, because in this case we would find fewer 'missing dependencies' issues.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 20, 2017

Contributor

@shindere - the testsuite modifying test is informational only (it's permitted to fail); the problem is the Changes file. We're frequently hitting GitHub API limits at the moment. I looked into it a few weeks ago - I've been meaning to talk to @avsm about setting up a mirror of the GPR labels on non-rate-limited infrastructure.

Contributor

dra27 commented Feb 20, 2017

@shindere - the testsuite modifying test is informational only (it's permitted to fail); the problem is the Changes file. We're frequently hitting GitHub API limits at the moment. I looked into it a few weeks ago - I've been meaning to talk to @avsm about setting up a mirror of the GPR labels on non-rate-limited infrastructure.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 21, 2017

Contributor
Contributor

shindere commented Feb 21, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Mar 1, 2017

Contributor

Just rebased this PR on trunk.

Is there anything missing for it to be merged?

Contributor

shindere commented Mar 1, 2017

Just rebased this PR on trunk.

Is there anything missing for it to be merged?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Mar 1, 2017

Contributor

I have one further thought (and sorry for its being late in the process): you have made the Makefiles consistent in not having a depend target for msvc (cf. otherlibs/str/Makefile which didn't and byterun/Makefile which did beforehand) but I wonder if this is the wrong path. At present, make alldepend fails on msvc with a cryptic error message because the depend target isn't defined.

In each case where there is

ifneq "$(TOOLCHAIN)" "msvc"

would it better to have:

ifeq "$(TOOLCHAIN)" "msvc"
$(error Dependencies cannot be regenerated using the MSVC ports)
else

Have you run a make alldepend since rebasing? I went through the differences caused by running make alldepend on mingw64. I think a lot may be that make alldepend was just missing, but here's the complete list:

asmrun/afl{,.p,.d,.i}.o
Order of dependencies changes (I'm not sure why this isn't stable - perhaps one make alldepend is enough?)

asmrun/str{,.p,.d,.i}.o
Some dependencies are listed twice, and several more on memory.h, major_gc.h, minor_gc.h, address_class.h, freelist.h appeared

asmrun/unix{,.p,.d,.i}.o DISAPPEARS

asmrun/win32{,.p,.d,.i}.o ADDED

Similar effect with str.o and afl.o in byterun/

byterun/win32.o dependencies gain flexdll.h (non-critical)

debugger/loadprinter.cmo gains dependency on ../parsing/location.cmi
debugger/loadprinter.cmx gains dependency on ../parsing/location.cmx

otherlibs/bigarray/.depend
Unix dependencies re-map from unix->win32unix

I wonder if a few of these are worthy of dealing with:

  • asmrun's depend target should always consider win32.o and unix.o
  • Perhaps a sed trick can map the dependency on unixsupport.h to the correct directory (UNIX_OR_WIN32 or something in config/Makefile, I think?)
  • The dependency on flexdll.h is awkward, but it should certainly be filtered in some way, as it would be problematic
Contributor

dra27 commented Mar 1, 2017

I have one further thought (and sorry for its being late in the process): you have made the Makefiles consistent in not having a depend target for msvc (cf. otherlibs/str/Makefile which didn't and byterun/Makefile which did beforehand) but I wonder if this is the wrong path. At present, make alldepend fails on msvc with a cryptic error message because the depend target isn't defined.

In each case where there is

ifneq "$(TOOLCHAIN)" "msvc"

would it better to have:

ifeq "$(TOOLCHAIN)" "msvc"
$(error Dependencies cannot be regenerated using the MSVC ports)
else

Have you run a make alldepend since rebasing? I went through the differences caused by running make alldepend on mingw64. I think a lot may be that make alldepend was just missing, but here's the complete list:

asmrun/afl{,.p,.d,.i}.o
Order of dependencies changes (I'm not sure why this isn't stable - perhaps one make alldepend is enough?)

asmrun/str{,.p,.d,.i}.o
Some dependencies are listed twice, and several more on memory.h, major_gc.h, minor_gc.h, address_class.h, freelist.h appeared

asmrun/unix{,.p,.d,.i}.o DISAPPEARS

asmrun/win32{,.p,.d,.i}.o ADDED

Similar effect with str.o and afl.o in byterun/

byterun/win32.o dependencies gain flexdll.h (non-critical)

debugger/loadprinter.cmo gains dependency on ../parsing/location.cmi
debugger/loadprinter.cmx gains dependency on ../parsing/location.cmx

otherlibs/bigarray/.depend
Unix dependencies re-map from unix->win32unix

I wonder if a few of these are worthy of dealing with:

  • asmrun's depend target should always consider win32.o and unix.o
  • Perhaps a sed trick can map the dependency on unixsupport.h to the correct directory (UNIX_OR_WIN32 or something in config/Makefile, I think?)
  • The dependency on flexdll.h is awkward, but it should certainly be filtered in some way, as it would be problematic
@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Mar 1, 2017

Contributor
Contributor

shindere commented Mar 1, 2017

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Mar 1, 2017

Contributor

To me, philosophically, it would be better for each Makefile which has a depend target to have a meaningful error about why it's not available, rather than the error - but I agree that just replacing alldepend in the root Makefile is indisputably simpler!

For running make alldepend, I'm not totally sure either: I think that these are to do with changes where the previous commits should themselves have run make alldepend.

Similarly with the mingw64 differences, I'm not sure if this is strictly for here, or just worth fixing at the same time. I guess it's not creating blocking problems, but the lack of win32.o dependencies, etc. isn't great?

Contributor

dra27 commented Mar 1, 2017

To me, philosophically, it would be better for each Makefile which has a depend target to have a meaningful error about why it's not available, rather than the error - but I agree that just replacing alldepend in the root Makefile is indisputably simpler!

For running make alldepend, I'm not totally sure either: I think that these are to do with changes where the previous commits should themselves have run make alldepend.

Similarly with the mingw64 differences, I'm not sure if this is strictly for here, or just worth fixing at the same time. I guess it's not creating blocking problems, but the lack of win32.o dependencies, etc. isn't great?

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Mar 3, 2017

Contributor
Contributor

shindere commented Mar 3, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Mar 7, 2017

Contributor

@dra27: the latest version of the PR tries to address your comments in
#1047 (comment)

In particular, make [all]depend now fails with a clear error message on
msvc.

The PR also makes an effort to compute dependencies for files although
they would not be compiled on the platform where make alldepend is run,
e.g. on a given platform not all libraries under otherlibs are compiled
but with this PR make alldepend will compute dependencies even for the
libraries that don't get compiled on this platform.

The last commit of the PR updates all the dependencies with the result
of make alldepend run on a Linux amd64 platform.

One thing to keep in mind is that make alldepend will continue to
produce platform-specific results, for the following reasons:

  • The architecture-specific files have different dependencies but the
    build system does not distinguish them from each other
    (could be fixed)

  • The order in which dependencies are computed varies slightly from
    platfom to platfom and so does their presentaiton. This of course has
    no effect on the dependency graph itself but introduces some noise in
    the diffs.
    (could likely be fixed)

  • More importantly: completely accurate dependencies can be known only
    at compile time. For instance, foo.c may depend on a.h or b.h
    depending on some preprocessor macro whose value is known for sure
    only at compile time.

To sum up: as long as the depencies will be stored statically in the
repository as is currently done, it will not be possible to have
completely accurate dependencies on all platforms. Given the limitations
inherent to this static approach, I'd suggest to not put too much energy in
improving it and just fix it when it stops working.

My personal preference would be that the dependencies are handled in a
completely dynamic way, but given that this problem is not really a
blocker (things more or less work, although not completely
satisfactory), I believe this is not really a high priority, at the moment.

Contributor

shindere commented Mar 7, 2017

@dra27: the latest version of the PR tries to address your comments in
#1047 (comment)

In particular, make [all]depend now fails with a clear error message on
msvc.

The PR also makes an effort to compute dependencies for files although
they would not be compiled on the platform where make alldepend is run,
e.g. on a given platform not all libraries under otherlibs are compiled
but with this PR make alldepend will compute dependencies even for the
libraries that don't get compiled on this platform.

The last commit of the PR updates all the dependencies with the result
of make alldepend run on a Linux amd64 platform.

One thing to keep in mind is that make alldepend will continue to
produce platform-specific results, for the following reasons:

  • The architecture-specific files have different dependencies but the
    build system does not distinguish them from each other
    (could be fixed)

  • The order in which dependencies are computed varies slightly from
    platfom to platfom and so does their presentaiton. This of course has
    no effect on the dependency graph itself but introduces some noise in
    the diffs.
    (could likely be fixed)

  • More importantly: completely accurate dependencies can be known only
    at compile time. For instance, foo.c may depend on a.h or b.h
    depending on some preprocessor macro whose value is known for sure
    only at compile time.

To sum up: as long as the depencies will be stored statically in the
repository as is currently done, it will not be possible to have
completely accurate dependencies on all platforms. Given the limitations
inherent to this static approach, I'd suggest to not put too much energy in
improving it and just fix it when it stops working.

My personal preference would be that the dependencies are handled in a
completely dynamic way, but given that this problem is not really a
blocker (things more or less work, although not completely
satisfactory), I believe this is not really a high priority, at the moment.

shindere added some commits Feb 16, 2017

Make .depend files generated for C sources more portable
This implements a suggestion provided by @xavierleroy in
#941 (comment)

Before this commit, the dependencies of the win32unix C files were not
taken into account at all. This commit fixes this, too.
asmrun/Makefile: dependencies generation fixup
When computing dependencies, make sure to take into account even the files
which will not actually be compiled / linked on the platform where
make depend is run.

Typically, this means compute dependencies also for ../byterun/win32.c
on Unix and vice versa.
Fix dependencies in otherlibs/systhreads
Before this commit, the dependencies computed for st_stubs were
useless because they were generated for st_stubs.o whereas what is actually
built is st_stubs_b.o and st_stubs_n.o.
Fix dependencies generation for otherlibs
Generate dependencies for all of them, not just those that will
actually be built.

@damiendoligez damiendoligez merged commit 9e81b0f into ocaml:trunk Mar 9, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment