Skip to content

Fix building sak with cross-compilers - #13804

Merged
gasche merged 5 commits into
ocaml:trunkfrom
MisterDA:sak-build
Feb 18, 2025
Merged

gasche merged 5 commits into
ocaml:trunkfrom
MisterDA:sak-build

Conversation

@MisterDA

Copy link
Copy Markdown
Contributor

The build and target compiler invocations may differ. For instance, if cross-compiling with build/host x86_64-*-linux and target x86_64-pc-windows, the build/host object extension is .o, but the target uses .obj.

The AX_PROG_CC_FOR_BUILD macro has fixes from revision 21 to 26 that are needed, or it would overwrite OBJEXT and EXEEXT with BUILD_OBJEXT and BUILD_EXEEXT without defining the latter.

A subtlety is that building on Windows for native Windows targets isn't cross-compiling, thus it's not the build compiler for sak (*-pc-cygwin/*-pc-msys) that is used, but the host/target compiler.

cc @shym

@gasche

gasche commented Feb 17, 2025

Copy link
Copy Markdown
Member

As a non-expert I looked at whether I could close my eyes and merge it right away. My assessment is "not quite":

  • the first two commits look fine and I would merge them
  • the third commit looks globally okay, and/but it is the first (unless I missed something) to introduce BUILD_* variables at the Makefile level. If I understand correctly, Simplify the build of cross compilers #13526 left Makefiles mostly unchanged (except Makefile.cross which is for advanced stuff), and now we are gradually introducing host/build/target distinctions into normal makefiles. I'm not opposed, but I wouldn't mind getting confirmation from Someone Knowledgeable (@dra27, @shindere, @shym) that this is the right way to go.

@shym shym left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • b5b1daa Prefer Autoconf EXEEXT to our exeext
    LGTM

  • b8cdedb Reformat an AS_IF
    LGTM

  • 034f8b7 Fix sak build not using the proper build compiler invocation
    I don’t understand what use cases this is fixing (I see the changes of extensions in cross-compiler setups but is there anything more than aesthetic considerations?).
    I agree that this is introducing quite some variables. To avoid the $(BUILD_O) for instance, I would rather go that way:

    • merge the rules for sak into one (separating compilation from linking is not really useful there),
    • define a do-it-all SAK_BUILD variable in Makefile.build_config.in set up during configure,
    • if the extension of the sak binary really matters (note that we often use .exe extensions on Unix...), define SAK also in configure/Makefile.build_config.in.

    But I like the idea of defining outputobj and outputexe at the same place in configure.ac!

@MisterDA

MisterDA commented Feb 17, 2025 •

Copy link
Copy Markdown
Contributor Author

I don’t understand what use cases this is fixing (I see the changes of extensions in cross-compiler setups but is there anything more than aesthetic considerations?).

$ clang-20  -Foruntime/sak.o -c runtime/sak.c
$ clang-20  -o runtime/sak runtime/sak.o
clang-20: error: no such file or directory: 'runtime/sak.o'
clang-20: error: no input files

I'm using here clang-20 for the build compiler, but with a *-pc-windows target (MSVC ccomp_type). You see that the build flags are mixed up: it's using -Fo.
The AX_PROG_CC_FOR_BUILD update fixes a confusion between the build and the target object and executable extensions.

I agree that this is introducing quite some variables. To avoid the $(BUILD_O) for instance, I would rather go that way:

  • merge the rules for sak into one (separating compilation from linking is not really useful there),
  • define a do-it-all SAK_BUILD variable in Makefile.build_config.in set up during configure,
  • if the extension of the sak binary really matters (note that we often use .exe extensions on Unix...), define SAK also in configure/Makefile.build_config.in.

I agree this could be cleaner, but if we build sak using MKEXE_VIA_CC, we can't pass it the list of cflags, because of the msvc syntax (cflags need to be passed before the /link flag). This would require some special casing or duplication… I'm scratching my head and I can't find a nice way.

I think dra27 has plans to remove sak, so introducing these build variables could be seen as temporary.

Autoconf detects `.exe` on all Windows targets. OBJEXT and EXEEXT are
automatically substituted.
The AX_PROG_CC_FOR_BUILD macro has fixes from revision 21 to 26 that
are needed, or it would overwrite OBJEXT and EXEEXT with BUILD_OBJEXT
and BUILD_EXEEXT without defining the latter.
@MisterDA

MisterDA commented Feb 18, 2025 •

Copy link
Copy Markdown
Contributor Author

I've managed to make a single invocation to build sak. MSVC still generates a sak.obj. That's a bit unfortunate. I have two proposals: one using the direct invocation: 232aa53, and one with build variables 9e09521 (it uses BUILD_OBJEXT/BUILD_O, BUILD_EXEEXT, build_outputexe and build_outputobj). My preference goes to using build variables.

The run-crosscompiler-tests label should be added to this PR

@shym shym left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for that update!
I prefer that version and it looks good to me, I have only minor stylistic suggestions.

Comment thread configure.ac Outdated
Comment thread configure.ac Outdated
sak doesn't need separate compilation and linking stages, and this
avoids adding build-specific variables to the Makefiles.

Co-authored-by: shym <samuel@tarides.com>

@gasche gasche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both!

@gasche
gasche merged commit dd47dcf into ocaml:trunk Feb 18, 2025
@MisterDA
MisterDA deleted the sak-build branch April 17, 2025 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants