Merge Unix and Windows build systems in the asmrun directory #941
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shindere
force-pushed
the
merge-asmrun-makefiles
branch
from
ad86d7b
to
34fbd0f
Compare
|
I think it's quite helpful for users if the runtime is always built with I seem to remember that the |
adrien-n
reviewed
Dec 2, 2016
| $(ARCMD) rc libasmrun.a $(OBJS) | ||
| $(RANLIB) libasmrun.a | ||
| ASPPFLAGS = -DSYS_$(SYSTEM) | ||
| ifeq "$(UNIX_OR_WIN32)" "unix" |
There was a problem hiding this comment.
I think we can drop the conditional and pass the define to all builds. I believe the only reason it is not currently passed is the lack of a need rather than the need to avoid it.
| all: libasmrun.a all-$(RUNTIMED) all-$(PROFILINGTARGET) all-$(SHARED) | ||
| FLAGS=\ | ||
| -I../byterun $(IFLEXDIR) \ | ||
| -DNATIVE_CODE -DTARGET_$(ARCH) -DMODEL_$(MODEL) -DSYS_$(SYSTEM) \ |
There was a problem hiding this comment.
I'm tempted to say that the sharing with ASPPFLAGS should be increased but I would prefer even more that they are provided through config/m.h instead. It's probably better to not add such changes now however since there are other things that are more important.
|
|
||
| all-noruntimed: | ||
| .PHONY: all-noruntimed | ||
| ifeq ($(TOOLCHAIN),msvc) |
There was a problem hiding this comment.
Can you make that consistent with other "ifeq" conditionals in the file (i.e. with arguments separated by spaces)? (I understand this is moved from the .nt part)
| rm -f libasmrund.a | ||
| $(ARCMD) rc libasmrund.a $(DOBJS) | ||
| $(RANLIB) libasmrund.a | ||
| OBJS=$(COBJS) $(ASMOBJS) |
There was a problem hiding this comment.
The lines that follow feel like they duplicate code but I support what you've done, i.e. longer but simpler code.
|
|
||
| $(LINKEDFILES): %.c: ../byterun/%.c | ||
| ln -s ../byterun/$*.c $*.c | ||
| $(LN) $< $@ | ||
| # $(LN) ../byterun/$*.c $*.c |
There was a problem hiding this comment.
Is this comment intentional? It helps parse the rule however but also made me wonder if some functionality had been disabled. A short comment explaining the .c files needed are the same as the ones in ../byterun would probably help understand this bit better (I regularly forget it exists).
| .PHONY: clean | ||
| clean: | ||
| rm -f $(LINKEDFILES) | ||
| rm -f *.$(O) *.$(A) *.$(SO) *~ |
There was a problem hiding this comment.
'*~' should be dropped from the list (I noticed it was there before but nonetheless, that's bad practice).
|
|
||
| ifeq "$(UNIX_OR_WIN32)" "win32" | ||
| .depend.nt: .depend | ||
| sed -e 's/\.o/.$(O)/g' .depend > .depend.nt |
There was a problem hiding this comment.
You can drop the 'g' in the sed command since there is only one match per line and that will make it consistent with the sed commands above (which only handle one match per line).
| depend: $(COBJS:.o=.c) ${LINKEDFILES} | ||
| ifneq "$(TOOLCHAIN)" "msvc" | ||
| .PHONY: depend | ||
| depend: $(COBJS:.$(O)=.c) $(LINKEDFILES) | ||
| $(CC) -MM $(FLAGS) *.c > .depend | ||
| $(CC) -MM $(FLAGS) -DPROFILING *.c | sed -e 's/\.o/.p.o/' >> .depend |
There was a problem hiding this comment.
The regex should probably be improved in a separate commit: '.o' could easily lead to a false positive. '.o:' would already be a clear improvement I think. Moreover it could be rewritten as 's/.o:/.p&/' ('\0' is a gnu-ism unfortunately; that doesn't mean I've checked my example works on BSD however).
There was a problem hiding this comment.
Note that it's not .o but \.o that is matched, and only the first one one the line. As for using & I think it would only make it harder to read.
Edit -- In fact I misread your comment because GitHub seems to be doing funny things with backslashes.
| $(call MKLIB,libasmrun.$(A), $(OBJS)) | ||
|
|
||
| i386nt.obj: i386nt.asm | ||
| $(ASM)i386nt.obj i386nt.asm |
There was a problem hiding this comment.
This indicates that this was probably written with msvc's command-line interface in mind. I'm not sure your new invocations are compatible with it however. To give you an idea on the possible matter:
config/Makefile.mingw64:ASM=$(TOOLPREF)as
config/Makefile.msvc64:ASM=ml64 -nologo -Cp -c -Fo
|
|
||
| ifeq "$(UNIX_OR_WIN32)" "win32" |
There was a problem hiding this comment.
Does anyone know if MSVC is usable to generate the .depend files? If so it would be possible to drop this completely simply by using "$(O)" above.
I would at least change the test to be on $(TOOLCHAIN) instead and I would name the file '.depend.msvc' instead.
|
Also, I'm not sure the runtime should be built with -O0 if -O2 brings a performance improvement because while it makes debugging crashes slightly more difficult, it usually doesn't make it impossible. Moreover the runtime is fairly stable nowadays. That said, if the topic hasn't been discussed yet, it should be done in a /separate/ discussion and the current behaviour should be kept. |
|
The |
|
The |
|
As a distributor, I like -g and stripping to a separate file that ends
up in a -dbg package. It helps a lot for situations where a user who
encounters an issue is certainly not going to get involved much if at
all (so basically all users).
I currently don't mind one way or the other in any case, I mostly wanted
to point out that use-case of distributions (and for instance, Debian is
moving to having -dbg packages built that way for every package).
|
shindere
force-pushed
the
merge-asmrun-makefiles
branch
8 times, most recently
from
c1a5606
to
56e05b5
Compare
|
@xavierleroy: for now, I'd suggest to drop both -g and -O0
Later, we can always make sure these flags can be passed somehow at
configure/compile time if desired.
Would this approach be okay?
|
|
First of all, many, many thanks @adrien-n for your very careful review.
I really appreciate it, especially given that this kind of PR is not
necessarily part of the most reviewed ones.
The branch has been updated and rebased. It starts with a commit that
does things in a rather conservative way, even when this makes the
resulting code not as nice / readable / factorized as it could be.
If there are things that should be changed compared to the behaviour of
trunk, I'd prefere to keep them in separate commits to make sure the
merge of build systems itself remains on its own.
So far I could check the following properties of this first commit.
On Linux, when configuring with -with-debug-runtime and
-with-instrumented-runtime,
make world.opt && make install
produces exactly the same installation, before and after the commit.
The only difference is in the installed Makefile.config, due to the
variable renamings that occured in the commit.
On Windows (mingw32 and msvc32), comparing the results of make instal
does not really make sense, because of the way the Windows linker works
(two executables will differ even if they have been produced by exactly
the same command, because Windows' linker inserts a timestamp into
them).
So for these ports, I rather compared the outputs of make world.opt
before and after the commit. The only significant difference is that
after the commit the PIC objects are built, whereas they were not
before. Except that, everything seems to work in a very similar way.
Now, coming to your remarks, @adrien-n :
I've noticed that "testsuite/tests/lib-dynlink-native/Makefile" refers
to SHARED so you'll have to update it.
Thanks, I missed that one. However, I ran the test in this directory
with:
.../ocaml/testsuite$ make one DIR=tests/lib-dynlink-native
And it turns out that the test passes both before and after the commit in
question.
The line where $(SHARE) appears says:
@cd sub; $(OCAMLOPT) -c $(SHARED) api.ml
I suspect this line is actually never executed, because $(SHARED) can be
either "share" or "noshare" and I assume the compiler would complain on
both of these.
So to conclude on this, although this line looks indeed suspiscious, I am
tempted to not touch it, since I don't know how to _really_ fix it. Any
suggestion is welcome. One other thing to decide seems to be whether
such a fix should be part of this PR or not.
>
-libasmrun.a: $(OBJS)
- rm -f libasmrun.a
- $(ARCMD) rc libasmrun.a $(OBJS)
- $(RANLIB) libasmrun.a
+ASPPFLAGS = -DSYS_$(SYSTEM)
+ifeq "$(UNIX_OR_WIN32)" "unix"
I think we can drop the conditional and pass the define to all builds.
I believe the only reason it is not currently passed is the lack of a
need rather than the need to avoid it.
Well to be honest I followed exactly the same line of thought. I still
decided to remain conservative. And also, if the flag does not make
sense, then I still think it is wiser not to pass it. I understand the
"it can't hurt" argument, but still... :)
Even if it can't hurt now, it seems to me that not passing unnecessary
defines may help to detect future errors in the code.
> endif
-all: libasmrun.a all-$(RUNTIMED) all-$(PROFILINGTARGET) all-$(SHARED)
+FLAGS=\
+ -I../byterun $(IFLEXDIR) \
+ -DNATIVE_CODE -DTARGET_$(ARCH) -DMODEL_$(MODEL) -DSYS_$(SYSTEM) \
I'm tempted to say that the sharing with ASPPFLAGS should be increased
but I would prefer even more that they are provided through config/m.h
instead. It's probably better to not add such changes now however
since there are other things that are more important.
Yes, I again agree with everything you said. IMO, this PR is already
rather big, not in terms of code size but rather in terms of changes it
introduces. So I think my preference would be to get it merged as soon
as possible, because its sole real purpose is to merge build systems. Then
once it's in trunk and thus becomes tested and used it will become
easier to make progresses from there.
In other words, that's already a big step so rather not make it even
bigger and let's go with that big step and then followup with smaller
steps, if necessary.
-all-noruntimed:
-.PHONY: all-noruntimed
+ifeq ($(TOOLCHAIN),msvc)
Can you make that consistent with other "ifeq" conditionals in the
file (i.e. with arguments separated by spaces)? (I understand this is
moved from the .nt part)
Done, thanks.
>
-libasmrund.a: $(DOBJS)
- rm -f libasmrund.a
- $(ARCMD) rc libasmrund.a $(DOBJS)
- $(RANLIB) libasmrund.a
+OBJS=$(COBJS) $(ASMOBJS)
The lines that follow feel like they duplicate code but I support what
you've done, i.e. longer but simpler code.
Are you talking about the variable definitions, i.e. DOBJS & co, or
about the way TARGETS is defined?
(thanks for the support anyway)
>
$(LINKEDFILES): %.c: ../byterun/%.c
- ln -s ../byterun/$*.c $*.c
+ $(LN) $< $@
+# $(LN) ../byterun/$*.c $*.c
Is this comment intentional?
No, I forgot to remove it, thanks!
It helps parse the rule however but also made me wonder if some
functionality had been disabled.
It has now been removed.
A short comment explaining the .c
files needed are the same as the ones in ../byterun would probably
help understand this bit better (I regularly forget it exists).
Really? It does not look that obscure to me...
-%.pic.o: %.c
- $(CC) -c $(PICFLAGS) -o $@ $<
+%.$(O): %.c
+ $(CC) $(CFLAGS) -c $<
Can you move the '-c' right after '$(CC)' so the argument order
matches with other rules?
Do you mind if this is postponded until we really work on the build
system?
Here the order of arguments has been kept as it was before, to simplify
the comparison of outputs of make world.opt...
>
-clean::
- rm -f *.o *.a *.so *~
+.PHONY: clean
+clean:
+ rm -f $(LINKEDFILES)
+ rm -f *.$(O) *.$(A) *.$(SO) *~
'*~' should be dropped from the list (I noticed it was there before
but nonetheless, that's bad practice).
I moved the ``rm -f *~'' command to the ``distclean'' rule. Is that
okay?
>
+ifeq "$(UNIX_OR_WIN32)" "win32"
+.depend.nt: .depend
+ sed -e 's/\.o/.$(O)/g' .depend > .depend.nt
You can drop the 'g' in the sed command since there is only one match
per line and that will make it consistent with the sed commands above
(which only handle one match per line).
Yeah that could probably be done. Again I took the commands as they
were, assumiing that all this would be changed later and that such kind
of improvement were not really part of this PR.
-depend: $(COBJS:.o=.c) ${LINKEDFILES}
+ifneq "$(TOOLCHAIN)" "msvc"
+.PHONY: depend
+depend: $(COBJS:.$(O)=.c) $(LINKEDFILES)
$(CC) -MM $(FLAGS) *.c > .depend
$(CC) -MM $(FLAGS) -DPROFILING *.c | sed -e 's/\.o/.p.o/' >> .depend
The regex should probably be improved in a separate commit: '\.o'
could easily lead to a false positive. '\.o:' would already be a clear
improvement I think. Moreover it could be rewritten as 's/\.o:/.p&/'
('\0' is a gnu-ism unfortunately; that doesn't mean I've checked my
example works on BSD however).
Okay, thanks. As you said, let's keep these for another commit.
> -
-ifeq ($(TOOLCHAIN),mingw)
-ASMOBJS=$(ARCH).o
-else
-ASMOBJS=$(ARCH)nt.obj
-endif
-
-OBJS=$(COBJS) $(ASMOBJS)
-
-all: libasmrun.$(A)
-
-libasmrun.$(A): $(OBJS)
- $(call MKLIB,libasmrun.$(A), $(OBJS))
-
-i386nt.obj: i386nt.asm
- $(ASM)i386nt.obj i386nt.asm
This indicates that this was probably written with msvc's command-line interface in mind. I'm not sure your new invocations are compatible with it however. To give you an idea on the possible matter:
config/Makefile.mingw64:ASM=$(TOOLPREF)as
config/Makefile.msvc64:ASM=ml64 -nologo -Cp -c -Fo
They were working but I changed them, agian to minimize the diff in the
outputs of make world.opt
>
+ifeq "$(UNIX_OR_WIN32)" "win32"
Does anyone know if MSVC is usable to generate the .depend files? If
so it would be possible to drop this completely simply by using "$(O)"
above.
I digged into this recently. msvc has a -showIncludes flag that could be
of some help. There are several issues with it, though. Some look minor
to me (the formatting is different and one file can be mentionned
several times with different paths). The most serious problem is that
all the includes are printed, even system ones. And so far, I could not
find a way to filter out the list to keep only the includes of interest.
Any suggestion here would be more than welcome. @dra27 suggested to use
gcc to compute dependencies, but as far as I remember nothing has been
decided regarding this.
I would at least change the test to be on $(TOOLCHAIN) instead and I
would name the file '.depend.msvc' instead.
I like the idea in principle, it makes perfectly sense. Could we
postpone it, though? Again to minimize changes in this PR and also
because there are other directories with the same kind of rule / file
naming conventions, and it would seem better to handle all of them in
one dedicated PR.
|
shindere
force-pushed
the
merge-asmrun-makefiles
branch
2 times, most recently
from
0eb20c8
to
4e84f84
Compare
|
A trick that I found useful in other projects: It is possible to have only one In the |
|
Nice trick! Thanks a lot @xavierleroy !
Can this change be done in another PR which would apply it to all the
directories where it applies and that would do just that?
|
shindere
force-pushed
the
merge-asmrun-makefiles
branch
2 times, most recently
from
0c828ff
to
01d2ebe
Compare
|
@shindere There is a significant regression on trunk at the moment (compared to 4.04) because the order of -O0 and -O2 options has changed, which I think is causing -O0 optimisation to be applied to the runtime, instead of -O2. Maybe you've fixed this already. (As an aside there may be another smaller performance regression which we will investigate once this one has been fixed.) |
|
(It looks like this has already been fixed; apologies if this mistakenly came from the Spacetime merge, although it had no effect in 4.04.) |
|
Mark Shinwell (2016/12/14 03:32 -0800):
@shindere There is a significant regression on trunk at the moment
(compared to 4.04) because the order of -O0 and -O2 options has
changed, which I think is causing -O0 optimisation to be applied to
the runtime, instead of -O2.
Sorry about that!
Maybe you've fixed this already.
The PR does actually remove the -O0, so yes I presume one can say it
fixes this issue.
(As an aside there may be another smaller performance regression which
we will investigate once this one has been fixed.)
If I can be of any help please do not hesitate to let me know.
|
|
On Thu, Dec 08, 2016, Sébastien Hinderer wrote:
On Windows (mingw32 and msvc32), comparing the results of make instal
does not really make sense, because of the way the Windows linker works
(two executables will differ even if they have been produced by exactly
the same command, because Windows' linker inserts a timestamp into
them).
GNU ld has --no-insert-timestamps which will avoid that difference. It
actually still insert a timestamp but a zero one.
Now, coming to your remarks, @adrien-n :
> I've noticed that "testsuite/tests/lib-dynlink-native/Makefile" refers
> to SHARED so you'll have to update it.
Thanks, I missed that one. However, I ran the test in this directory
with:
.../ocaml/testsuite$ make one DIR=tests/lib-dynlink-native
And it turns out that the test passes both before and after the commit in
question.
The line where $(SHARE) appears says:
@cd sub; $(OCAMLOPT) -c $(SHARED) api.ml
I suspect this line is actually never executed, because $(SHARED) can be
either "share" or "noshare" and I assume the compiler would complain on
both of these.
So to conclude on this, although this line looks indeed suspiscious, I am
tempted to not touch it, since I don't know how to _really_ fix it. Any
suggestion is welcome. One other thing to decide seems to be whether
such a fix should be part of this PR or not.
I'm not very familiar with the testsuite since I haven't had that much
time to fiddle with the compiler since it was introduced. I would still
do the text change within this commit and afterwards do any fix that is
deemed appropriate as part of a separate set of commits.
I usually have a preference towards introducing another commit to do the
fix first but if we take that route, we might never finish the original
changes.
> -libasmrun.a: $(OBJS)
> - rm -f libasmrun.a
> - $(ARCMD) rc libasmrun.a $(OBJS)
> - $(RANLIB) libasmrun.a
> +ASPPFLAGS = -DSYS_$(SYSTEM)
> +ifeq "$(UNIX_OR_WIN32)" "unix"
>
> I think we can drop the conditional and pass the define to all builds.
> I believe the only reason it is not currently passed is the lack of a
> need rather than the need to avoid it.
Well to be honest I followed exactly the same line of thought. I still
decided to remain conservative. And also, if the flag does not make
sense, then I still think it is wiser not to pass it. I understand the
"it can't hurt" argument, but still... :)
Even if it can't hurt now, it seems to me that not passing unnecessary
defines may help to detect future errors in the code.
OK. I have a slight preference for passing it anyway but I'm absolutely
not opposed as doing it this way instead.
> -all: libasmrun.a all-$(RUNTIMED) all-$(PROFILINGTARGET) all-$(SHARED)
> +FLAGS=\
> + -I../byterun $(IFLEXDIR) \
> + -DNATIVE_CODE -DTARGET_$(ARCH) -DMODEL_$(MODEL) -DSYS_$(SYSTEM) \
>
> I'm tempted to say that the sharing with ASPPFLAGS should be increased
> but I would prefer even more that they are provided through config/m.h
> instead. It's probably better to not add such changes now however
> since there are other things that are more important.
Yes, I again agree with everything you said. IMO, this PR is already
rather big, not in terms of code size but rather in terms of changes it
introduces. So I think my preference would be to get it merged as soon
as possible, because its sole real purpose is to merge build systems. Then
once it's in trunk and thus becomes tested and used it will become
easier to make progresses from there.
In other words, that's already a big step so rather not make it even
bigger and let's go with that big step and then followup with smaller
steps, if necessary.
OK. Again, fine by me.
> -libasmrund.a: $(DOBJS)
> - rm -f libasmrund.a
> - $(ARCMD) rc libasmrund.a $(DOBJS)
> - $(RANLIB) libasmrund.a
> +OBJS=$(COBJS) $(ASMOBJS)
>
> The lines that follow feel like they duplicate code but I support what
> you've done, i.e. longer but simpler code.
Are you talking about the variable definitions, i.e. DOBJS & co, or
about the way TARGETS is defined?
(thanks for the support anyway)
I was talking about the various *OBJS variables. I was tempted to make
something more concise but got something complex and not much shorter. I
wanted to record my support of the "dumb" approach here in case someone
started to support a more "clever" approach instead.
> A short comment explaining the .c
> files needed are the same as the ones in ../byterun would probably
> help understand this bit better (I regularly forget it exists).
Really? It does not look that obscure to me...
It's not obscure but I often forget that the build creates these
symlinks. I'm not particularly attached to having a comment there
though: do as you feel like.
> -%.pic.o: %.c
> - $(CC) -c $(PICFLAGS) -o $@ $<
> +%.$(O): %.c
> + $(CC) $(CFLAGS) -c $<
>
> Can you move the '-c' right after '$(CC)' so the argument order
> matches with other rules?
Do you mind if this is postponded until we really work on the build
system?
Here the order of arguments has been kept as it was before, to simplify
the comparison of outputs of make world.opt...
No problem.
> -clean::
> - rm -f *.o *.a *.so *~
> +.PHONY: clean
> +clean:
> + rm -f $(LINKEDFILES)
> + rm -f *.$(O) *.$(A) *.$(SO) *~
>
> '*~' should be dropped from the list (I noticed it was there before
> but nonetheless, that's bad practice).
I moved the ``rm -f *~'' command to the ``distclean'' rule. Is that
okay?
Thanks.
> +ifeq "$(UNIX_OR_WIN32)" "win32"
> +.depend.nt: .depend
> + sed -e 's/\.o/.$(O)/g' .depend > .depend.nt
>
> You can drop the 'g' in the sed command since there is only one match
> per line and that will make it consistent with the sed commands above
> (which only handle one match per line).
Yeah that could probably be done. Again I took the commands as they
were, assumiing that all this would be changed later and that such kind
of improvement were not really part of this PR.
OK, no hurry. And I'm even more in favor of a change in the spirit of
what Xavier mentioned.
> -depend: $(COBJS:.o=.c) ${LINKEDFILES}
> +ifneq "$(TOOLCHAIN)" "msvc"
> +.PHONY: depend
> +depend: $(COBJS:.$(O)=.c) $(LINKEDFILES)
> $(CC) -MM $(FLAGS) *.c > .depend
> $(CC) -MM $(FLAGS) -DPROFILING *.c | sed -e 's/\.o/.p.o/' >> .depend
>
> The regex should probably be improved in a separate commit: '\.o'
> could easily lead to a false positive. '\.o:' would already be a clear
> improvement I think. Moreover it could be rewritten as 's/\.o:/.p&/'
> ('\0' is a gnu-ism unfortunately; that doesn't mean I've checked my
> example works on BSD however).
Okay, thanks. As you said, let's keep these for another commit.
OK.
> > -
> -ifeq ($(TOOLCHAIN),mingw)
> -ASMOBJS=$(ARCH).o
> -else
> -ASMOBJS=$(ARCH)nt.obj
> -endif
> -
> -OBJS=$(COBJS) $(ASMOBJS)
> -
> -all: libasmrun.$(A)
> -
> -libasmrun.$(A): $(OBJS)
> - $(call MKLIB,libasmrun.$(A), $(OBJS))
> -
> -i386nt.obj: i386nt.asm
> - $(ASM)i386nt.obj i386nt.asm
>
> This indicates that this was probably written with msvc's command-line interface in mind. I'm not sure your new invocations are compatible with it however. To give you an idea on the possible matter:
>
> config/Makefile.mingw64:ASM=$(TOOLPREF)as
> config/Makefile.msvc64:ASM=ml64 -nologo -Cp -c -Fo
They were working but I changed them, agian to minimize the diff in the
outputs of make world.opt
As long as the MSVC build still works, I'm fine with everything in this
domain. Or rather, I'm not competent to judge this.
> +ifeq "$(UNIX_OR_WIN32)" "win32"
>
> Does anyone know if MSVC is usable to generate the .depend files? If
> so it would be possible to drop this completely simply by using "$(O)"
> above.
I digged into this recently. msvc has a -showIncludes flag that could be
of some help. There are several issues with it, though. Some look minor
to me (the formatting is different and one file can be mentionned
several times with different paths). The most serious problem is that
all the includes are printed, even system ones. And so far, I could not
find a way to filter out the list to keep only the includes of interest.
Any suggestion here would be more than welcome. @dra27 suggested to use
gcc to compute dependencies, but as far as I remember nothing has been
decided regarding this.
OK. That's clearly an improvement so we can see this later on.
> I would at least change the test to be on $(TOOLCHAIN) instead and I
> would name the file '.depend.msvc' instead.
I like the idea in principle, it makes perfectly sense. Could we
postpone it, though? Again to minimize changes in this PR and also
because there are other directories with the same kind of rule / file
naming conventions, and it would seem better to handle all of them in
one dedicated PR.
OK again, especially considering Xavier's comment.
Thanks!
…--
Adrien Nader
|
shindere
force-pushed
the
merge-asmrun-makefiles
branch
from
01d2ebe
to
93aa21f
Compare
|
GNU ld has --no-insert-timestamps which will avoid that difference. It
actually still insert a timestamp but a zero one.
Thanks! I think the real issue is with the msvc port which as far as I
understand it uses Microsoft's linker.
> Now, coming to your remarks, @adrien-n :
>
> > I've noticed that "testsuite/tests/lib-dynlink-native/Makefile" refers
> > to SHARED so you'll have to update it.
>
> Thanks, I missed that one. However, I ran the test in this directory
> with:
>
> .../ocaml/testsuite$ make one DIR=tests/lib-dynlink-native
>
> And it turns out that the test passes both before and after the commit in
> question.
>
> The line where $(SHARE) appears says:
>
> @cd sub; $(OCAMLOPT) -c $(SHARED) api.ml
>
> I suspect this line is actually never executed, because $(SHARED) can be
> either "share" or "noshare" and I assume the compiler would complain on
> both of these.
>
> So to conclude on this, although this line looks indeed suspiscious, I am
> tempted to not touch it, since I don't know how to _really_ fix it. Any
> suggestion is welcome. One other thing to decide seems to be whether
> such a fix should be part of this PR or not.
I'm not very familiar with the testsuite since I haven't had that much
time to fiddle with the compiler since it was introduced. I would still
do the text change within this commit and afterwards do any fix that is
deemed appropriate as part of a separate set of commits.
Done. I also rebased on latest trunk.
I usually have a preference towards introducing another commit to do the
fix first but if we take that route, we might never finish the original
changes.
Yes, exactly. I try to do the same in general, but here I feel a bit
less concerned because I know this is something I will have to deal with
in the future anyway, as part of migrating tests to ocamltest.
The same holds for several of your (fully justified) remarks regarding
the build system. That's something I know I will work on later, that's
why I am not so much in a hurry about making things as cleanly as
possible right now, especially when this means a bigger change compared
to how things worked before.
I think the build system can be improved in many ways and that at least
some of these improvements diserve their own PRs and should take into
account the whole build system rather than just one directory as this PR
does.
> > -libasmrund.a: $(DOBJS)
> > - rm -f libasmrund.a
> > - $(ARCMD) rc libasmrund.a $(DOBJS)
> > - $(RANLIB) libasmrund.a
> > +OBJS=$(COBJS) $(ASMOBJS)
> >
> > The lines that follow feel like they duplicate code but I support what
> > you've done, i.e. longer but simpler code.
>
> Are you talking about the variable definitions, i.e. DOBJS & co, or
> about the way TARGETS is defined?
>
> (thanks for the support anyway)
I was talking about the various *OBJS variables. I was tempted to make
something more concise but got something complex and not much shorter. I
wanted to record my support of the "dumb" approach here in case someone
started to support a more "clever" approach instead.
I see. KISS. :) I'm pretty sure something can be done e.g. to build
libraries, like for instance introducing a more sophisticated make
function. But that falls into what I have described above.
> > +ifeq "$(UNIX_OR_WIN32)" "win32"
> > +.depend.nt: .depend
> > + sed -e 's/\.o/.$(O)/g' .depend > .depend.nt
> >
> > You can drop the 'g' in the sed command since there is only one match
> > per line and that will make it consistent with the sed commands above
> > (which only handle one match per line).
>
> Yeah that could probably be done. Again I took the commands as they
> were, assumiing that all this would be changed later and that such kind
> of improvement were not really part of this PR.
OK, no hurry. And I'm even more in favor of a change in the spirit of
what Xavier mentioned.
It is a nice idea, indeed. But as I understand it, it still requires gcc
to generate dependencies. So it will indeed simplify the build system
because it lets one generate one .depend file that works for all ports,
but it won't let us generate dependencies dynamically on msvc, which I
find a pity.
As long as the MSVC build still works, I'm fine with everything in this
domain. Or rather, I'm not competent to judge this.
As far as I could test, it still works.
|
Let me be very clear here: I don't want msvc to generate the |
This makes things closer to what they were before, since PIC objects were not built for Windows.
shindere
force-pushed
the
merge-asmrun-makefiles
branch
from
32b493a
to
d2138ce
Compare
|
Just rebased. |
|
I see that Damien has been doing some work on the CI - has this been through precheck, just to check for any subtleties on other arches? |
|
David Allsopp (2016/12/20 00:36 -0800):
I see that Damien has been doing some work on the CI - has this been
through precheck, just to check for any subtleties on other arches?
Not yet but I just did the push to the precheck repo. Thanks!
|
|
OK - if Damien's changes aren't complete (or aren't yet copied to precheck), it's possible that you'll see complete failure on all 6 Windows ports, but I can always test them again manually! |
|
David Allsopp (2016/12/20 01:49 -0800):
OK - if Damien's changes aren't complete (or aren't yet copied to
precheck), it's possible that you'll see complete failure on all 6
Windows ports, but I can always test them again manually!
Thanks. Actually precheck passed on all windows ports.
|
damiendoligez
approved these changes
Dec 20, 2016
| @@ -2182,7 +2177,7 @@ else | |||
| inf "Source-level replay debugger: not supported" | |||
| fi | |||
|
|
|||
| if test "$debugruntime" = "runtimed"; then | |||
| if test $debugruntime; then | |||
There was a problem hiding this comment.
The call to test is redundant here because true and false are shell commands.
|
David Allsopp (2016/12/19 01:42 -0800):
dra27 commented on this pull request.
> else
-PROFILINGTARGET = noprof
+LN = ln -s
endif
A future note for when you get to autoconf:
https://github.com/dra27/dose/blob/master/configure.ac#L261-L269
Interesting, thanks!
Sébastien.
|
|
It's got even more interesting since the next version of Windows 10 allows developers to use symlinks without elevating! |
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Feb 16, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment)
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Feb 17, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment) Before this commit, the dependencies of the win32unix C files were not taken into account at all. This commit fixes this, too.
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Feb 17, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment) Before this commit, the dependencies of the win32unix C files were not taken into account at all. This commit fixes this, too.
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Feb 21, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment) Before this commit, the dependencies of the win32unix C files were not taken into account at all. This commit fixes this, too.
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Mar 1, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment) Before this commit, the dependencies of the win32unix C files were not taken into account at all. This commit fixes this, too.
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Mar 3, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment) Before this commit, the dependencies of the win32unix C files were not taken into account at all. This commit fixes this, too.
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Mar 7, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment) Before this commit, the dependencies of the win32unix C files were not taken into account at all. This commit fixes this, too.
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Mar 7, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment) Before this commit, the dependencies of the win32unix C files were not taken into account at all. This commit fixes this, too.
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Mar 8, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment) Before this commit, the dependencies of the win32unix C files were not taken into account at all. This commit fixes this, too.
shindere
added a commit
to shindere/ocaml
that referenced
this pull request
Mar 9, 2017
This implements a suggestion provided by @xavierleroy in ocaml#941 (comment) Before this commit, the dependencies of the win32unix C files were not taken into account at all. This commit fixes this, too.
damiendoligez
pushed a commit
that referenced
this pull request
Mar 9, 2017
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.
camlspotter
pushed a commit
to camlspotter/ocaml
that referenced
this pull request
Oct 17, 2017
* Merge Unix and Windows build systems in the asmrun directory
Changes in make variables:
- Removal of the SHARED make variable, which had the same
semantics than SUPPORTS_SHARED_LIBRARIES, the later having values true
and false while the former had values shared and noshared.
(SHARED was not defined on Windows)
- RUNTIMED now takes values true and false rather than runtimed and
noruntimed
* Do not use -O0 in asmrun's Makefile
* Add /asmrun/win32.c to .gitignore
* Build PIC libraries only under Unix
This makes things closer to what they were before, since PIC objects
were not built for Windows.
EmileTrotignon
pushed a commit
to EmileTrotignon/ocaml
that referenced
this pull request
Jan 12, 2024
better 404 page on package documentation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cc @adrien-n @alainfrisch @dra27 @whitequark
(Is there somebody else who could / should be Cc'ed?)
For the Makefiles in asmrun themselves the diff seems difficult to read
so reviewers may prefere to read the new asmrun/Makefile directly, given
that asmrun/Makefile.nt just includes the asmrun/Makefile and
asmrun/Makefile.shared has been removed.
It is worth noting that, for the moment, CFLAGS contains -g -O0 as it
did before. Shouldn't these two options be in DFLAGS rather than CFLAGS?