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

adapt build system for cross-compilation #5737

Closed
vicuna opened this issue Aug 23, 2012 · 91 comments
Closed

adapt build system for cross-compilation #5737

vicuna opened this issue Aug 23, 2012 · 91 comments

Comments

@vicuna
Copy link

@vicuna vicuna commented Aug 23, 2012

Original bug ID: 5737
Reporter: william
Status: assigned (set by meyer on 2012-12-30T03:13:09Z)
Resolution: open
Priority: normal
Severity: feature
Platform: mxe
OS: linux
Version: 3.12.1
Category: configure and build/install
Tags: patch
Related to: #6613
Monitored by: J5lx william @glondu @xclerc Camarade_Tux kerneis jm @hcarty

Bug description

Hello,
I am using mxe on linux to cross-compile my application for windows. It uses camlimages, lablgtk2, lablgl, cairo, xml-light (and ocamlbuild+findlib)
Mxe is a platform for cross-compilation, it includes patches and make commands for hundreds of packages.
http://mxe.cc/ (main site)
https://github.com/mxe/mxe (sources)
https://github.com/william3/mxe (fork being integrated)

To build ocaml I adapted debian patches maintained by Romain Beauxis, to make it usable with many different targets (i686-pc-mingw32, i686-w64-mingw32, x86_64-w64-mingw32 for 32/64bits targets).

Though it works fine with ocaml 3.12.1, I would like :

  • to get your feed back if possible, to know if the process could be simplified (I do not really understand the patch)
  • to know if you could integrate part of the code for next versions

Best regards,
William

File attachments

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 4, 2012

Comment author: @gasche

I have no authority on the question of whether such patches could be accepted in a future state. I just had a look at the patches related to the OCaml compiler proper (very quick, so feel free to correct misunderstandings): I must they they're not really that convincing.

I would understand their integration in a source tree designed solely for unix-to-windows compilation, with the understanding that they will remain as a fork forever. Even then, I would be a bit wary of this integration of all those independent changes in a single patch, making potential errors/divergences more difficult to track (that seems unrelated to your work and inevitable under the current project organization, though; they should consider moving to a structure with multiple patches per project).
But integrating them upstream is clearly impossible in their current state.

If I were a maintainer I wouldn't touch this until the changes are sorted out and given more structure and justification.

I also noticed that a lot of the "ocaml"-related patches do not concern the ocaml compiler proper: cairo, findlib, camlimages, xml-light, etc. are third-party libraries. If you have not yet contacted the upstream of those projects, you should consider doing so. A post on the caml-list may also interest people that would learn about how to make their programs portable with respect to this specific situation and, one can always hope, motivate a few charitable souls to help you getting the patches in better shape?

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 4, 2012

Comment author: william

Hello,
Thanks for this analysis. My questions were only targeting this:
https://github.com/william3/mxe/blob/master/src/ocaml-core-1-fixes.patch

Indeed, I tried to submit other patches to related people, but did not get many answers...

I would appreciate any hint that would improve the ocaml related patch :
https://github.com/william3/mxe/blob/master/src/ocaml-core-1-fixes.patch
which is the patch given above :
http://caml.inria.fr/mantis/file_download.php?file_id=731&type=bug

Changes to ocamlbuild were made mainly in order to make ocamlbuild usable for cross targets with ocamlfind integration.

Regards,
William

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 26, 2012

Comment author: Camarade_Tux

Hi,

Based on William's patch, I made a series of smaller patches.

First one is: 0001-config-Makefile.mingw64-remove-redundant-mms-bitfiel.patch (and last one is 13th). The patch 0002 is something I forgot (I should have moved it out and not submit it). The patchs have some details if you select "show content" so I won't repeat it here. Some patches are trivial but for others, I have some questions on the approach.

  • 0003-configure-use-stderr-for-all-messages-targetted-at-t.patch
    Some things were printed to stdout and some others to stderr; I now send everything to stderr.

  • 0010, 0011, 0013:
    Are they a "right" approach? These changes need some polishing but is it ok to add special cases for cross-compilation with hardcoded settings?

Thanks,
Adrien Nader

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 28, 2012

Comment author: meyer

Hi,

Thanks for the patches. It looks like it's only a build system.

I reviewed them all, my comments below, however haven't attempted to cross compile OCaml yet. I might try it for ARM this weekend.
I think we need at least a second pair of eyes to look at the patches because they look really promising!

Comments

  • patch 1

looks good apart from the part it would be good to:

  • document why we are disabling the -mms-bitfields and what kind of
    implications it has

  • maybe making it configurable, like a variable for cross compilation
    that is empty

  • patch 2; ignored, spurious .gitignore/vim stuff

  • patch 3

I admire simplicity and portability here, but we should use automatic redirection or custom command. Then if somebody adds another echo, she would be forced to use 'msg' command instead of echo and redirection I can see however that we use it in all the places, so looks Ok.

  • patch 4
    Looks OK.

  • patch 5
    Why do we need to touch Makefile.nt?

but looks OK. With a reservation it will be tested on Windows.

  • patch 6
    Looks OK.

  • patch 7

I trust this cases remained the same, so looks ok.

  • patch 8

Same as above.

  • patch 9
    Looks a bit hairy.
    First of all why just not substitute host for target before it's been used?

It's certainly improvement to use it but increases a risk, of breaking some exotic builds.

  • patch 10
    We should initialise TOOLPREF in the begining if we plan to use it.

We should be able to replace these patterns:
-gcc
with something like:
gcc
thus avoiding a lot of redundant pattern matches

it might cause troubles only when something like 'mygcc' was used which is fairly unrelastic.

I think better way of using TOOLPREF would be to, once it's defined, redefine the interesting tools (as & gcc) to ${AS} & ${GCC} with TOOLPREF prefix and then use these variables.

  • patch 11

This:

  • x86_64--mingw) echo "Wow! A 64 bit architecture!" 1>&2
  •                 echo "#undef ARCH_SIXTYFOUR" >> m.h
    

doesn't sound like a win situation.

It will be more consistent to use variables like ${TOOLCHAIN}. Instead of ${toolchain} but that's just cosmetic.

I've gone through all the mingw cases, but can't guarantee they are correct until I would see myself ocaml crosscompiling.

  • Patches 12,13
    Looks OK.

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 29, 2012

Comment author: Camarade_Tux

Thanks for the review.

So far I'm not cross-compiling OCaml successfully. By posting them here I'm trying to get the overall approach validated. There are also some changes that can be merged independently of the others and doing so will reduce the size of subsequent patches (including out-of-tree ones in the case cross-compilation support cannot be included easily).

I've updated the set of patches and I'm going to post them inside a tarball now or this page will soon be unreadable. I've reorganized the patches to move the least intrusive and least dangerous ones first so the patch numbers have changed but in this comment I'll use the same numbers as you did.

The file is named "cross-patches-rev2-2012-12-29.tar.gz".

I've dropped the patch about -mms-bitfields. This switch is now enabled by default for -w64-mingw. However this changed only happened in gcc 4.7.

I've introduced inf()/wrn()/err() functions. inf() writes its arguments to stderr; wrn() also writes "WARNING:\n" before; err() also "exit 2".

Forward slashes work just as well on windows. I've changed Makefile.nt to use forward slashes in advance. I'm not sure yet whether I'll be able to use the regular Makefiles or if I'll have to rely on Makefile.nt ones. I've moved the patch down in the list; we'll see a bit later.

Regarding "host" vs. "target" in the configure file, I simply started and went over all the cases using "$host". It turned out that all of them actually want the "target". However it's quite likely "host" will also be needed (probably for ocamlobjinfo for instance: it runs on $host and needs to link to a C library).

I think better way of using TOOLPREF would be to, once it's defined, redefine the interesting tools (as & gcc) to ${AS} & ${GCC} with TOOLPREF prefix and then use these variables.

I've tried to do that but it touches areas for which I'm not sure I understand the intent. The biggest use of ${TOOLPREF}gcc is around line 830 and starts with:
case "$arch,$system" in
amd64,macosx) as="${TOOLPREF}as -arch x86_64"
aspp="${TOOLPREF}gcc -arch x86_64 -c";;
As far as I understand, ocaml requires aspp to come from gcc in this setup. Was that the intent or is it because the only compiler available was gcc? And what about clang/llvm?

I need some more input on that.

I've fixed the "#undef ARCH_SIXTYFOUR" issue and I've replaced "toolchain" with "TOOLCHAIN".

edit: I've fixed two issues but I'm not uploading another archive right now:
first,

if target_type is "unknown", the script will try to cross-compile for the "unknown" architecture

-target_type=unknown
+target_type=""

second,
inf/wrn/err functions need to invoke echo with "-e" and need to quote the arguments

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 30, 2012

Comment author: meyer

Thanks for the patches, here are my further comments:

  • patch 1

I like the cleanup you have made for DBM. I've applied this patch.

  • patch 2

Looks good. Applied, thanks.

  • patch 3

Not applied.

What kind of problems compatibilty.h gives?

If there are problems we should fix it in the header.

  • patch 4

I like the attempt you've made to improve the ./configure script.

I'm also quite reserved about redirecting everything to stderr. Your
patch probably just changes how the user will interact with the script
as instead the user will filter the messages by string which might be
better or not.

By any means, I am tempted to apply these changes, once I know if there
are mandatory, thanks.

  • Patch 5

I am resistant to warn the user, would be better to support both
notations? If you decide to do this, please do it for all the flags.

  • patch 6

Is incorrect.

cd config/auto-aux/

cc=gcc sh ./runtest sizes.c

4 4 4 2

This will print to stdout the sizes of the primitive types.

Quotation will make them spliced into set builtin shell command, thus
setting the arguments to each of the sizes.

However you have issued:

ret=$?

set $ret

which just sets the first argument to the return code of the command.

  • Patch 7

I'm resistant, looking closely I think it would be a premature
optimisation.

It's a small improvement that buys nothing, and might be reverted in a
future. Is there any particular reason for this cleanup?

  • Patch 8

target_type is initially set to "unknown" therefore we shouldn't check
it with test -n. Also, we should print un-coditionaly information about
target, consistency here is more important. I also think we shouldn't
check-in mechanical renaming changes from $target to $host before we
have working cross-compilation so I'd refrain from checking-in the patch
now

  • patch 9

Follow up of 4, so skipped for time being.

  • patch 10

I wouldn't drop "support" for old buggy gcc versions, however in general
I am in favour of some cleanups.

Follow up of 4 so not applied yet.

  • patch 11

Looks all right, applied.

  • patches 12, 13

Don't have mingw system to test it, so I would rather not apply it
before I know it's important to change.

Overall, thank you for your efforts,

Wojciech

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 30, 2012

Comment author: Camarade_Tux

  • patch 3

The renaming from stat_alloc() to caml_stat_alloc() comes from William's patch. I haven't had an issue myself and thinking more about it, I'm not sure what would have caused an issue for William. I saw it as an opportunity for a slight update. I'm going to remove it for now; if "namespaced" function names are preferred, it will probably be better to change all of them and not only stat_alloc(), and do it at once. But for now, removed.

  • patch 4

I started redirecting messages to stderr because there were some blocks of code that I wanted to use as functions (and therefore have them write to stdout (or stderr or anything)) but which were already writing to stdout. Usage between stderr and stdout was inconsistant but stderr seemed to prevail so I settled for that.

I think that I don't have the original issue anymore and I don't mind where the messages are sent as long as it's consistent. Progress messages are probably better on stdout but if warnings and errors go to stderr, they will probably in the wrong place or out of context. So, everything to stdout?

I think I'll try to redirect inf/wrn/err to "3" or something like that and use the "exec" command to redirect 3 to stdout. That should free stdout and stderr for use in other places and display properly in the terminal. (I'll see how it goes)

It could be good to use colour too for wrn and err but doing it in a portable fashion might be difficult. For later.

  • patch 5

I added the check for "-option=val" vs. "-option val" because I made the mistake myself. The check is easy to implement but I'm not sure the support for "=" is. I'll see if '=' can be added to $IFS. However, it seems standard not to use '=' for ocaml

  • patch 6

Very good catch. I'm wondering if the issue can be solved with "set". If not, I'll probably introduce size_short/size_long/size_... variables.

  • patch 7

The unused variable in the case/in/esac constructs did not cause me any issue so far. I'll drop it.

  • patch 8

I caught the issue with target initially being "unknown" but only after making the archive. I've fixed it by making the default "" instead of "unknown".

I'll make it always print the target.

  • patch 10

I haven't dropped support for gcc 2.96/2.95. I've simply not touched them. This means the warnings won't appear if you cross-compile with gcc 2.96/29.5, which I consider an unlikely scenario.

Thanks a lot.

@vicuna
Copy link
Author

@vicuna vicuna commented Jan 23, 2013

Comment author: Camarade_Tux

Hi,

I've attached a new file. This is close to a code dump but I that way I'm somehow creating a backup for the patches. It's not really worth looking at them right now since they need cleaning.

@vicuna
Copy link
Author

@vicuna vicuna commented Jan 29, 2013

Comment author: meyer

Dear Adrien,

Sorry to keep you waiting, I'm going to look at your cross-compilation patches this week.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 17, 2013

Comment author: Camarade_Tux

I've add "cross-patches-rev2-2013-02-17.tar.gz" (still "rev2" by mistake). I still need to clean things a bit but...

ocamlopt.opt works!

There's one thing I'm not verry happy with: I had to do changes asmcomp/ because it uses nativeints for many integers and I build on a 64bit system for a 32bit one; I ended up with several bad integer litterals in the generated assembly. This will definitely need proofreading.

Also, at first I had tried to cross-compile things like ocamldoc while building the cross-compiler. That didn't make much sense (that would only make sense if I were cross-compiling the compiler). In the end I added the ability to disable building ocamldoc and a few others like it is doable for camlp4.
So I did something that will improve cross-compiling the compiler but that is not needed for this bug report. What should I do about these patches?

Try to have them in? Try to have them but after the other ones? Forget about them (for now)?
Depending on the answer I'll reorganize the order of my patches to match so wait for rev4 before reading these patches.

Thanks. :-)

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 18, 2013

Comment author: meyer

Thanks for all the patches. My initial comments below, I said I applied

  • but just realised after reviewing 31 small patches that there are 40 of
    them, so I'll postpone it for tomorrow. The general comment is that it's
    getting into the right track. Some of the patches needs a really
    thorough reviewing before we commit them. The ones I marked with a
    star are the ones that I think needs some improvements. I'm updating
    this in-place as it goes along with applying and testing them. Short
    note: did you try your cross-compile patches on a different
    architecture, i.e ARM? Also we might try porting them to OPAM.
    The reason why I ask is that I do have an ARM device, and would like to
    build packages for it, and probably many other people would like.

Patch 1/
I don't think we should keep flexdll.h it in the triangle quotes - it's
not a system header.
However I think it's a part of OCaml installation so we should apply this patch.

Patch 2/
I trust it works, however I am not able to test it.

Patch 3/
Looks fine, thanks. I am not sure if we should prefix it with
underscore, my initial thought was that we shouldn't, but it's a header
not used anywhere else so maybe not worth to change.

Patch 4/
Looks OK to me, applied.

Patch 5/*
The echo -e command prints on my machine also leading -e.

$ ./configure --prefix $PWD
-e Configuring for host i686-pc-linux-gnu ...
-e Configuring for target i686-pc-linux-gnu ...
-e Using compiler gcc.

also the message prefix breaks the line:

./configure -help
ERROR:
-e Unknown option "-help".

The script continues to run even with error:

-e Using compiler arm-linux-gnueabi-gcc.
./tst: 1: Syntax error: word unexpected (expecting ")")
WARNING:
-e Unable to compile the test program.
-e This failure is expected for cross-compilation: we will assume the C compiler is ANSI-compliant.
-e Checking the sizes of integers and pointers...

Patch 6/
It's always best to inform the user about his failure. Applied.

Patch 7/
Applied. Thanks.

Patch 8/
I don't think we need $model anymore, so thanks for cleaning this up. I've applied this patch too.

Patch 9/
This needs to be documented. Probably additional entries to the manual
needs to be done. Especially that we are able to cross compile now at all.

Patch 10/
We should use conventional lowercase variable names in the Caml build
system. TOOLPREF and TOOLCHAIN all are not obeying the convention.

I suspect TOOLPREF/TOOLCHAIN are a mingw specific variable, but can't
find it anywhere. If we decide to expose it, then it needs to be
documented - I propose in the configure script itself as a commentary. But I'd leave it as it is.

My worry here is that we are hardcoding the sizes, instead of detecting
them.

I'll apply this patch however, because can't think about anything better
at the moment.

Patch 11/
I like this patch, applied. Did you try to test it on a different shell than bash?

Patch 12/*
I don't like this: '# XXX' prefix for the comment.

I'd prefer nicer prefix, like TODO or FIXME. However it's not a show
stopper, patch looks complicated so need to make a sweep over this
again.

Patch 13/
I don't have mingw system to test it, and there is 32 how about 64? If
there is another flavour of the library we need to cover that. I
tentatively applied.

Patch 14/
Looks OK.

Patch 15/*
ocaml -version can print other characters than specified in the sed command.

It would be much safer if the you matched for example like this:
sed 's/.version (.)/\1/g'

Also:
if test x"$target" != x"$host"; then
we should be using ==.

Patch 16/
Makes sense, applied.
It looks like a continuation of the previous patch, it would be easier to collate them.

Patch 17/
Looks OK, applied.

Patch 18/
Looks good to me, applied.

Patch 19/
Looks OK, applied.

Patch 20/
Fine. Applied.

Patch 21/
Apart from visible whitespaces it, looks OK.

EDIT: Sorry, I notice that the file have a lot leading whitespaces, I'll
clean them up as they polluting (not necessarily) the file.

Patch 22/
Thank you, applied.

Patch 23/
Don't put comma after `if' condition. Otherwise applied.

Patch 24/*
To be reviewed.

Patch 25/*
Yet to be reviewed again.

Patch 26/
Looks good to me. Thanks.

Patch 27/*
Needs to be reviewed.

Patch 28/*
I like it, but needs more looking.

Patch 29-40
Needs to be reviewed.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 18, 2013

Comment author: meyer

Try to have them in? Try to have them but after the other ones? Forget about them (for now)?
Depending on the answer I'll reorganize the order of my patches to match so wait for rev4 before reading these patches.

We could consider them as an extra set of patches, if you separate them nicely, it would be a nice addition, but be careful to not over complicate what is now.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 18, 2013

Comment author: Camarade_Tux

Thanks for the comments.

I'll reorganize the commits so that the patches that could benefit cross-compilation of the compiler appear last.
I'll need a few more days to do that. Probably this week-end.

Btw, for TOOLPREF, it's not specific to anything. It's either arbitrary or already somewhere in the ocaml sources.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 18, 2013

Comment author: meyer

Absolutely no rush, I'm going to look at it again in few days time - I have a fully booked week too.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 19, 2013

Comment author: william

about Camarade_Tux comment from 2013-02-17 22:57, regarding ocamldoc and camlp4 :

There are two categories of executables :

the ones that need to be cross compiled :
ocamlc ocamlrun ocamlcp ocamldep ocamlmklib ocamlmktop ocamlopt ocamlprof

the ones that do not need to be cross compiled :
camlp4prof camlp4boot camlp4 camlp4oof camlp4of camlp4o camlp4rf camlp4r camlp4orf ocamldoc ocamllex ocamlyacc

For all of them, it is important to be able to have them "prefixed", like
i686-w64-mingw32-ocamlc, even if this is for a program such as ocamldoc that is not really cross compiled. Because when building other libraries and programs, it is easier to use prefixed tools no matter if it is ocamldoc or ocamlc.

So would it be possible to have a build system that preserves building of those tools ?

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 19, 2013

Comment author: Camarade_Tux

Well, it's difficult to have them build during the same build.

I believe it should be a requirement that if you want to build a cross ocaml X.Y.Z, you need to have a native ocaml X.Y.Z on your system.
The reason is fairly simple: there's no guarantee that it'll work otherwise. You might try with a different version of the compiler but you'll need a native ocaml compiler that is still fairly close version-wise. (plus you need the native compiler for camlp4 and ocamlbuild to work along with a working Dynlink)

That means that in order to build the cross-compiler, you will have to have a matching native toolchain. In that case you will be able to reuse most of it to get the right tools.

If you need to have specific names for these tools too, simply 'cp' or 'ln -s' them. They will exist for sure during build: they're really needed.

It might be doable to have them built as native tools during the build of the cross-compiler but it's really much more work. That will basically require that it is possible to cross-compile a native compiler. For now, 'cp' or 'ln -s' are very good ways to do it and actually the configure of the cross-compiler will fail if the version of the native compiler and the version of the cross-compiler you're building don't match.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 19, 2013

Comment author: meyer

Dear William and Adrien,

Maybe a small chime in: building a cross-compiler does not preclude requirement of not having the compiler on the path. It does however simplify a lot the build process.

In general the problem could be solved by a separate recursive step for Makefile, so in normal world OCaml is built like this:

  • bootstrap compiler builds bytecode compiler
  • bytecode compiler builds bytecode compiler and the native one
  • native compiler builds a native compiler compiled natively

in the cross-compilation world additional recursive invokation of Makefile would happen with all the variables set up to met the requirement building the cross-compiler.

In practice, it's not that simple however and I don't think it's needed at a cost of making it more complex. So I'd assume we already have the native toolchain already in place.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 20, 2013

Comment author: Camarade_Tux

I forgot to mention that both gfortran and gnat have the same requirement for cross-compilers and that's why I stopped trying to not require a native toolchain.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 20, 2013

Comment author: william

Indeed you are right.
In mxe, I ended up building first native ocaml then cross ocaml, copying native version of ocamldoc and camlp4* with right prefix.
Do not make things more complex, it is fine like that.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 23, 2013

Comment author: meyer

I tried your patches with arm toolchain on x86:

$ ./configure --target arm-linux-gnueabi 
-e Configuring for host i686-pc-linux-gnu ...
-e Configuring for target arm-linux-gnueabi ...
-e Using compiler arm-linux-gnueabi-gcc.
./tst: 1: Syntax error: word unexpected (expecting ")")
WARNING:
-e Unable to compile the test program.
-e This failure is expected for cross-compilation: we will assume the C compiler is ANSI-compliant.
-e Checking the sizes of integers and pointers...
./tst: 1: Syntax error: word unexpected (expecting ")")
ERROR:
-e Unable to compile the test program.
 Make sure the C compiler 'arm-linux-gnueabi-gcc -O ' is properly installed.

2 things:

  • tag prefix breaks lines - I included this comment in the summary
  • the configure script continues running even though it failed - this was included in the previous-previous comments: exit will return from the shell function not from the script.

Also I would suggest testing on both on mingw and any other popular architecture (ARM for instance).

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 23, 2013

Comment author: william

could you please explain how to patch sources with cross-patches-rev2-2013-02-17.tar.gz ?
I know the command :
patch -p1 < /path/to/patch
but how to use it for 30 patches ? I tried using "find /path/to/patch-dir -exec patch -p1 < {} ;", but does not work
Thanks

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 24, 2013

Comment author: meyer

William,
if you are sitting in the git repo, using git svn, then this command just works:

git am *.patch

if not than:

for f in *.patch; do patch -p1 < $f; done

(not that < {} will not work as it will splice two halves to redirection)

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 24, 2013

Comment author: william

based on the git directory :
git clone http://github.com/ocaml/ocaml.git
I do :
git am ../cross-patches-rev2-2013-02-17/*.patch

and get :
Applying: byterun/win32.c: use < > to #include flexdll.h instead of "".
Applying: windows: don't define lseeki64 and lseek since it already exists.
[...]
/home/performance/src/mxe/gits/ocaml/.git/rebase-apply/patch:16: space before tab in indent.
debugger.o meta.o dynlink.o
warning: 1 line adds whitespace errors.
Applying: yacc: "ocamlyacc$(EXE)" rule produced "ocamlyacc" (no trailing $(EXE)).
Applying: .gitignore: ignore vim temp files and .cm* stuff.
Applying: STASH
error: patch failed: build/boot.sh:15
error: build/boot.sh: patch does not apply
Patch failed at 0037 STASH

same problem with git version of 17/02/2013

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 24, 2013

Comment author: Camarade_Tux

Here's a new version of the patches. It's against HEAD~ (damn people commiting on sundays!) but with the patch in #5887. The file is "cross-patches-rev4-2013-02-24.tar.gz".

I've moved commits around and I've squashed some of them together and got 31 commits (down from 40 or so).

There are three directories inside the archive:

  • 01-non-specific/ : 10 commits
  • 02-safe/ : 8 commits
  • 03-wip/ : 13 commits

Their names should be fairly explicit. The 03-wip directory is included for completeness but is not mergeable.

If moved the error messages from "echo -e" to "printf '%b\n'" since echo isn't portable:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html
"It is not possible to use echo portably across all POSIX systems unless both -n (as the first argument) and escape sequences are omitted."

The fact that there is a newline after "WARNING:" and "ERROR!" is on purpose: I've found it to look nicer and be easier to stop. Actually, I've added a new line before the tag and another one after the error message.

I've improved some messages. In particular, the error you had with the arm toolchain is because the patches have a hardcoded list of integers/pointers/shorts/longs sizes when cross-compiling since they cannot be guessed. Currently, only windows 32bit and 64bit are there. I prefer to leave someone else add the entries for other toolchains.

I've documented -target in the INSTALL file.

The TOOLPREF/TOOLCHAIN variables are mostly arbitrary: they appear like that in some other projects but there is absolutely no rule about them. I haven't changed their name in these patches but there is no reason to not do it if it is wished.

I've tried the configure script with ash too (instead of bash invoked as sh) and had no issue.

About the use of the "ws2_32" library, the 32 is both for 32bit and 64bit. Windows 64 provides the "win32" API. It's a name, they could also have named it "windows-api-after-dos" and it's not going away nor changing in incompatible ways any time soon.

I've replaced the invocation of "ocamlrun -version" with "ocamlc -version":
% ocamlrun -version
The Objective Caml runtime, version 3.12.1
% ocamlc -version
3.12.1

That made it possible to replace sed with cut (in order to strip the +dev... part in the version).

For patch 15, you said:
Also:
if test x"$target" != x"$host"; then
we should be using ==.
I don't understand: what is the issue?

For patch 23, you said:
Don't put comma after `if' condition. Otherwise applied.

I've replaced:
ifeq ($(var1), $(var2))
with:
ifeq "$(var1)" "$(var2)"

Is it what you had in mind?

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 24, 2013

Comment author: Camarade_Tux

Actually I've borked the change in byterun/Makefile.

I wrote:
ifeq $(SYSTEM) mingw
but it should have been:
ifeq "$(SYSTEM)" "mingw"

The corresponding file is 02-safe/0014-build-select-win32-variants-of-unix-and-graph-for-mi.patch .

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 25, 2013

Comment author: meyer

Adrien, thanks for the next round of patches.

My answers for your questions:

For patch 23, you said:
Don't put comma after `if' condition. Otherwise applied.

My bad typo, I actually meant: don't put space after the comma. The space after comma is significant. Correct syntax is:
ifeq ($(SYSTEM),mingw)

I'd not use cut, sed seems to be better tool for that, but you are right if we want just base version than probably the regex you submitted is OK.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 25, 2013

Comment author: meyer

Adrien,

I tested and committed first 5 patches from 01-non-specific/.

Please excuse doing that piece by piece, but I am also reviewing them during the tour. The next commits will contain the rest of the patches from 01-non-specific.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 27, 2013

Comment author: meyer

Adrien,

A small update:

Sorry, as usually, I got very limited means of time, to commit and test the patches, but in essence I really like what you did with sub-dividing them. I will do my best to commit the rest from 01-non-specific/ during the week so we can move on with the rest of the patches. I will look if the compilation actually worked on anything else than mingw, like ARM.

@vicuna
Copy link
Author

@vicuna vicuna commented Mar 8, 2013

Comment author: meyer

Patch 6/

inf() {
  printf "%b\n" "$*" 1>&3
}

wrn() {
  printf "\nWARNING:\n" 1>&3
  printf "%b\n\n" "$*" 1>&3
}

err() {
  printf "\nERROR!\n" 1>&3
  printf "%b\n\n" "$*" 1>&3
  exit 2
}

exec 3>&1

First you are writing to a custom descriptor 3, and then you will redirect everything from 3 to stdout (descriptor 1). What's the purpose?

Patch 7/
Modified patch looks good:

-if echo $configure_options | grep -q ' --\?[a-zA-Z0-9-]\+='; then
+if echo "$configure_options" | grep -qe '--\?[a-zA-Z0-9-]\+='; then

two things:

  • added missing quotes
  • and -e flag to escape leading --, and dropped the space

Patch 8/
Not applied.

Sporious lines here:

+  amd64,macosx)   as='as -arch x86_64'
+                  aspp='gcc -arch x86_64 -c';;

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 26, 2013

Comment author: Camarade_Tux

Soooo. Back on this.

First you are writing to a custom descriptor 3, and then you will redirect everything from 3 to stdout (descriptor 1). What's the purpose?

The reason is that the patch also does:

  • 4,4) echo "OK, this is a regular 32 bit architecture."
  • 4,4) inf "OK, this is a regular 32 bit architecture."
    echo "#undef ARCH_SIXTYFOUR" >> m.h

If stdout was used, the output from inf would pollute the one from echo. I prefer to make the function safer by also avoiding redirecting to stderr directly.

I've checked the commit that was merged as rev 13312 and then reverted by Damien with rev 13456. It factored some code so that the right ranlib was called for some step.
I believe it is not needed since the RANLIB variable will be updated by another patch to include the $(TOOLPREF) prefix and "ar" doesn't need to have "$(TOOLPREF)".

I'll update patch 7 to reflect your comments

For patch 8, I don't have the spurious lines you mention. I'll continue after I have some sleep.
edit: ah, seems to be patch 9 and I see it

@vicuna
Copy link
Author

@vicuna vicuna commented Aug 26, 2013

Comment author: Camarade_Tux

Here's another patch: 0001-build-replace-build-mk-config-myocamlbuild_config-.s.patch

I've had to remove mkconfig.sh and mkmyocamlbuild_config.sh since they prevented me from doing other changes and I couldn't change them. Plus the approach wasn't the best.

The next big change will be the removal of ocamlcomp.sh and the use of something more flexible which will make it possible to use the system compilers.

@vicuna
Copy link
Author

@vicuna vicuna commented Aug 28, 2013

Comment author: william

Adrien,
Please find in cross-build_20130829.tar.gz the actual state of Makefile and patches that I use with success to do cross-compilation. It takes into accound some of the improvements made so far, and so work with a quite recent trunk version of ocaml.

Could you please have a quick look at it, and give me hints on what I could do to simplify things further ?

@vicuna
Copy link
Author

@vicuna vicuna commented Aug 30, 2013

Comment author: meyer

Your patch is very welcome.

Few remarks from Damien:

  • we should not use \0 in sed pattern matches, that does not work on Mac OS X.
  • we should not be using [] in tr patterns.
  • $() notation for splicing shell commands might not work on non Bash
  • make -j3 is probably a left over

otherwise looks good!

and comments from me:

  • --no-print-directory might not work on BSD make

I attached modified patch, with exception for fix of --no-print-directory flag, has that been tested on BSD system?

@vicuna
Copy link
Author

@vicuna vicuna commented Aug 31, 2013

Comment author: Camarade_Tux

Thanks for the feedback.

  • we should not use \0 in sed pattern matches, that does not work on Mac OS X.
    Very stupid mistake from me; I'm sure I had the same issue a bit earlier.
  • we should not be using [] in tr patterns.
    Actually, that's fine: [a-z] [A-Z] will replace [ with [, a-z with A-Z and ] with ]. The use of brackets is from System V while BSD didn't require them.
    I wrote the commands with brackets and they looked weird; I find that using [:lower:] [:upper:] looks better (more readable)

$() notation for splicing shell commands might not work on non Bash
I've spent quite a lot of time trying to understand that one. SUSv2 requires $() and advises its use instead of backquotes.
I'm under the impression that it's csh and tcsh which don't handle $() while the bourne shells do. (t)csh would never be used as /bin/sh so it should be safe (if someone uses them as /bin/sh, he deserves the pain).
Are there shells that we know don't handle $() ? It's something I've been wondering for years.

  • make -j3 is probably a left over
    definitely, because of ...
  • --no-print-directory might not work on BSD make
    it's a very late change after I've noticed that make's output of "entering/leaving directory ..." would get mixed with the config.sh and myocamlbuild_config.ml files. A fairly stupid change on my side which would have broken with BSD make.
    I'm going to try to understand what's going on properly.

@vicuna
Copy link
Author

@vicuna vicuna commented Aug 31, 2013

Comment author: meyer

t's a very late change after I've noticed that make's output of >"entering/leaving directory ..." would get mixed with the config.sh and
myocamlbuild_config.ml files. A fairly stupid change on my side which would
have broken with BSD make.

So the most portable way to fix it would be to pattern match against the output you expect using grep, to filter out the messages from the Makefiles.

@vicuna
Copy link
Author

@vicuna vicuna commented Aug 31, 2013

Comment author: Camarade_Tux

I've uploaded 0001-build-replace-build-mk-config-myocamlbuild_config-.s-2.patch

It uses & instead of \0 for sed. It uses [:lower:] and [:upper:] instead of [a-z] and [A-Z] for tr. It keeps $(). --no-print-directory has been removed and instead the MAKEFLAGS and MAKELEVEL variables are unset in the shell script.

This has been tested on Linux and FreeBSD. Windows should be like Linux since it's built with GNU userland too.

@vicuna
Copy link
Author

@vicuna vicuna commented Aug 31, 2013

Comment author: @protz

I maintain the OCaml installer for Windows. If you want me to try one (or several) of your patches, let me know which ones, and I'll give it a try on Monday :).

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 1, 2013

Comment author: Camarade_Tux

I'm fairly certain this particular change won't break on Windows (I have a VM available but I've avoided starting it).

That said, more testing is definitely welcome and even more for the windows installer. I don't have the next patches ready in advance since that would mean updating them for trunk very often and repeatedly but testing the current trunk (and with the mkconfig/myocamlbuild_config patch if possible) would already be great. Thanks.

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 1, 2013

Comment author: Camarade_Tux

William, it's fairly difficult to tell you which patch would still be needed and even more because you have patches that only apply to you/mxe. Instead I can tell you which patches I still have to get integrated.

Before I forget about it, there's the bug 5887.
And, in random order:

  • asmcomp-internals-store-integers-as-Int64.patch
  • chose which ocamldoc to use (so that the system one is used)
  • remove the runocamldoc script which is replaced by something else
  • reove ocamlcomp.sh which is replaced by somethingn else more flexible
  • chose the ocamlc/ocamlopt/ocamlrun commands to use: use either the ones from the system (to build the cross-compiler) or the cross-compiling ones (to build the libraries for the cross-compiler)

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 7, 2013

Comment author: Camarade_Tux

New patches. :)

Archive file is cross-patches-2013-09-07.tar.gz .

The archive contains a new version of the last patch: on Windows, the ml code from the config makefiles has to be at the end of the file because it refers to other variables.

  • build: replace build/mk{config,myocamlbuild_config}.sh.

Also, something had to break on Windows for stupid reasons: an unused variable "DO" in the config makefiles, turned into "let do = ..." in OCaml, which gives a syntax error. Well, frustrating and fixed:

  • config/Makefile.{mingw,msvc}{,64}: remove unused DO variable.

I've also been running full tests on Windows, FreeBSD and Linux: configure, build, make, install, testsuite.

I found a few issues, notably that ocamldoc.opt was not built anymore on Windows:

  • build: ocamldoc.opt was not built anymore on Windows.

The new big patch is "build: replace ocamlcomp*.sh.".
It's a big one which touches lots of files. "ocamlcomp.sh" had limitations and hadn't been used everywhere. My patch provides something more flexible and updates chunks of code that weren't using "ocamlcomp.sh".

This patch requires a new boot/myocamlbuild.boot file. It's described in the commit message but I've also added a specific patch so that the myocamlbuild.boot update is not forgotten.

  • Regenerate a new boot/myocamlbuild.boot by copying _build/myocamlbuild.

Then, 3 patches make it possible to chose the ocamldoc command to run during the build since for cross-compilation, ocamldoc won't be built and we'll have to use the one from the system.

  • ocamldoc/Makefile: remove unneeded OCAMLDOC and OCAMLDOC_OPT variables.
  • ocamldoc: in configure, chose the "ocamldoc" command to use.
  • ocamldoc/runocamldoc: remove it; it's unused now.

Finally, a cosmetic patch:

  • byterun/Makefile: remove unneccessary -DCAML_NAME_SPACE in compile flags
    The functions in the corresponding files use the "caml_"-prefixed versions, making it useless to -DCAML_NAME_SPACE which purpose, afaiu, is to make it possible to call "alloc()" instead of "caml_alloc()".

edit: this leaves only very few changes left: #5887, using int64 instead of nativeint in asmcomp, introduce variables to distinguish between host and target compilers, and make sure we use the right commands everywhere. (and a couple more misc changes)

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 14, 2013

Comment author: Camarade_Tux

I've pushed cross-patches-2013-09-07-fixed.tar.gz .

  • rebased against current trunk (post-labltk removal)
  • fix for build/partial-install.sh not installing where the user asked to (save $BINDIR and friends before sourcing config.sh and restore them afterwards if they're not empty)

Discard the previous tarball and the two previous patches: this new tarball includes everything.

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 20, 2013

Comment author: Camarade_Tux

The jenkins build bots have uncovered three issues with the most recent patches.

  • "make clean" fails until ./configure has been run again
  • openbsd's make doesn't have -C (but freebsd has btw)
  • an issue with improper quoting of variables that is apparent on Windows with the "C:\Program Files (x86)" path: at least parens get interpreted

I've uploaded fixes for the first two issues. This leaves the last one which I'll tackle on tomorrow.

The patches have been tested on gnu/linux with make and with pmake (which is the parent to the bsd makes) and also on openbsd. The two patches are available as:

0001-build-fix-make-clean-failure-when-.-configure-hasn-t.patch
0002-build-don-t-use-make-s-C-it-s-not-available-everywhe.patch

Sorry for the breakage.

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 21, 2013

Comment author: Camarade_Tux

I've uploaded a new patch to fix the issue on Windows:
001-build-skip-IFLEXDIR-in-mk_shell_and_ocamlbuild_confi.patch

I have only tried it on my machine but it doesn't introduce anything new: it simply adds a variable to a blacklist and matches better the blacklist of build/mk{config,myocamlbuild_config}.sh.

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 21, 2013

Comment author: meyer

Thank you applied.

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 22, 2013

Comment author: Camarade_Tux

I've uploaded two patches:

0001-build-ocamlmklib-on-Windows-expect-a-Windows-style-p.patch
0002-build-typo-on-config-Makefile.mingw64-msvc-and-msvc6.patch

They fix the current issues noticed on the jenkins build bots.

Basically, the first one adds a call to "cygpath -m" because ocamlmklib is a Win32 executable and requires Win32 paths.

The second one fixes a change I had done in config/Makefile.mingw but skipped in config/Makefile.{mingw64,msvc,msvc64}.

@vicuna
Copy link
Author

@vicuna vicuna commented Sep 30, 2013

Comment author: @garrigue

Small bug in trunk: ocamlbuild uses ocamlc.opt even if it isn't the newest compiler.
I.e. the logic to switch between the bytecode and native versions of ocaml doesn't apply to ocamlbuild.
It had me scratching my head for a few hours while fixing a bug in type inference that occurred only in ocamlbuild.

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 1, 2013

Comment author: Camarade_Tux

Hi,

I'm really sorry for this issue. There is indeed clearly an issue in myocamlbuild.ml. I'll have it a go at it this evening.

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 1, 2013

Comment author: Camarade_Tux

I spent some time on the issue and something doesn't feel right in myocamlbuild.ml.

I can't understand why the "native_deps" list in the code below refers to .cmxa, .cmx and .o files. Unless I'm mistaken, these files won't be used during the compilation or link process.
Instead the lists should only contain "ocamlc(.opt)" and .cma and .cmo files.

The corresponding code:
let ocamlc_solver =
let native_deps = ["../ocamlc.opt"; "../stdlib/stdlib.cmxa";
"../stdlib/std_exit.cmx"; "../stdlib/std_exit"-.-C.o] in
let byte_deps = ["../ocamlc"; "../stdlib/stdlib.cma";
"../stdlib/std_exit.cmo"] in
if Pathname.exists "../ocamlcomp.sh" then S[A"../ocamlcomp.sh"] else
if List.for_all Pathname.exists native_deps then
S[A"./ocamlc.opt"; A"-nostdlib"]
else if List.for_all Pathname.exists byte_deps then
S[ocamlrun; A"./ocamlc"; A"-nostdlib"]

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 1, 2013

Comment author: meyer

Adrien,

at the moment we should not care too much about myocamlbuild.ml as we don't use it normally, and these scripts are planned to be removed in the future.

I'm committing your patches at the moment.

Please note that there are bootstrapping problems at the moment on trunk, e.g. the ocamllex used to build fresh ocaml is being build with the new ocaml and not the bootstrapped version.

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 1, 2013

Comment author: Camarade_Tux

Well, something has to be changed to fix the aforementionned issue. I believe the patch will be quite simple but I need to understand why using the opt-compiled bytecode compiler requires to have .cmx(a) files around.
It should simply be a matter of shelling out to call "test ocamlc.opt -nt ocamlc" (shelling out because Unix and therefore Unix.stat aren't available).

As for other patches, I'm not sure which ones you have in mind.

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 1, 2013

Comment author: meyer

currently these ones are pending:

0004-build-allow-UNIXDIR-to-be-win32unix.patch
0005-build-replace-build-mk-config-myocamlbuild_config-.s.patch
0006-build-replace-ocamlcomp-.sh.patch
0007-ocamldoc-Makefile-remove-unneeded-OCAMLDOC-and-OCAML.patch
0008-ocamldoc-in-configure-chose-the-ocamldoc-command-to-.patch
0009-ocamldoc-runocamldoc-remove-it-it-s-unused-now.patch
0010-byterun-Makefile-remove-unneccessary-DCAML_NAME_SPAC.patch

I'm going to spend some time on it, over the week, probably I will get Damien to review them.

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 2, 2013

Comment author: Camarade_Tux

Hmm, in this list, patches up to 6 (included) at least are already in.

That said, we should probably wait for one or two weeks for the current things to settle before continuing.

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 3, 2013

Comment author: Camarade_Tux

I've uploaded one file:
0001-myocamlbuild.ml-only-build-with-.opt-compilers-if-th.patch

Also, there is no need to consider previous patches in mantis; I will take care of reuploading updated versions of the ones that haven't been applied.

The patch fixes the issue reported above by Jacques and the second one fixes bootstrapping issues Alain Frisch has encountered.

I've checked that the .cmx/cmxa/o files that ocamlbuild was looking for were not required by chown root:root + chmod 640; the build process didn't fail unlike when I applied that to the .cmo file.
The only check is now native exists + native is more recent than byte-compiled.

PS: I really wish I could remove some of the files I've uploaded (found out they were broken right after uploading)

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 3, 2013

Comment author: @gasche

Sorry for not having any time to look at the ocamlbuild issue.

Re. faulty uploads: just send me a precise list of stuff to remove, here or by email. I haven't followed the discussion recently but I can still click "Delete".

@github-actions
Copy link

@github-actions github-actions bot commented May 15, 2020

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 15, 2020
@github-actions github-actions bot closed this Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant