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

Remove source preprocessing and simplify configuration #94

Merged
merged 13 commits into from
Jan 19, 2021

Conversation

xavierleroy
Copy link
Contributor

@xavierleroy xavierleroy commented Jan 4, 2021

This PR is a "reboot" of #86 and #87.

Now that per-target asm files are gone and OCaml 4.04 is the minimal version supported, we can remove a number of configuration parameters and avoid the preprocessing of z.mli and z.ml.

The version number can also be handled without preprocessing, as shown in #87.

The corresponding features are available in all OCaml versions we support,
so use them unconditionally.
The attribute `[@@noalloc]` is available in all OCaml versions we support.
Also: remove the generated `z_features.h` file, now always empty.
The version number is now extracted from META into an auxiliary
zarith_version.ml file.
@antoinemine
Copy link
Collaborator

Thanks!

configure still has some leftover code we could remove for asm configuration and flags (-noasm, -host, asopt, ASFLAGS).

In project.make, grep '^version' assumes a specific indentation of the META file (there are two version lines, only one of them has no space before). A grep 'version' | head -1 could fix this (nitpick).

- Remove the asm-related flags and variables (-noasm, asopt, ASFLAGS).
- Simplify architecture handling (-host, "arch" variable).
@xavierleroy
Copy link
Contributor Author

Right, thanks for the quick feedback. I pushed two commits that implement your suggestions.

configure Outdated
Comment on lines 304 to 323
# Add extra options for some hosts

arch='none'
case $host in
x86_64-*linux-gnu|x86_64-kfreebsd-gnu)
ccdef="-DZ_ELF -DZ_DOT_LABEL_PREFIX $ccdef"
if test $wordsize = 32; then
# 32-bit OCaml on a 64-bit host
arch='i686'
ccopt="-m32 $ccopt"
asopt="-m32 $asopt"
else
arch='x86_64'
fi
;;
i486-*linux-gnu|i686-*linux-gnu|i486-kfreebsd-gnu)
ccdef="-DZ_ELF -DZ_DOT_LABEL_PREFIX $ccdef"
arch='i686';;
i686-*cygwin)
if test "x$wordsize" = "x64"; then
ccdef="-DZ_COFF $ccdef"
arch='x86_64_mingw64'
else
ccdef="-DZ_UNDERSCORE_PREFIX -DZ_COFF $ccdef"
arch='i686'
fi
;;
i386-*darwin* | x86_64-*darwin*)
ccdef="-DZ_UNDERSCORE_PREFIX -DZ_MACOS $ccdef"
if test "x$wordsize" = "x64"; then
ccopt="-arch x86_64 $ccopt"
asopt="-arch x86_64 $asopt"
arch='x86_64'
checkcc
else
ccopt="-arch i386 $ccopt"
asopt="-arch i386 $asopt"
arch='i686'
checkcc
fi
;;
armv7*-gnueabi)
arch='arm'
;;
none)
;;
*)
echo "unknown host $host";;
esac

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this switch that sets ccopt in some cases may not be needed any longer. Since we compile .c files via ocamlc, architecture-specific flags such as -m32 or -arch x86_64 are already provided by ocamlc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK.
If needed in some circumstances, it is always possible to do CFLAGS=... ./configure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I went ahead and removed this case. Let's see what CI says, esp. on macOS.

Copy link
Collaborator

@antoinemine antoinemine left a comment

Choose a reason for hiding this comment

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

I think that ar is no longer needed..

No need to add "-m32" or "-arch x86_64" C compiler options:
normally, OCaml adds them when calling the C compiler to compile C files.
@antoinemine
Copy link
Collaborator

I think we could merge this. Unless you see further simplifications?

@xavierleroy
Copy link
Contributor Author

OK, merging now!

@xavierleroy xavierleroy merged commit b229acf into master Jan 19, 2021
@xavierleroy xavierleroy deleted the simplified-configuration branch January 19, 2021 13:13
@hannesm hannesm mentioned this pull request Feb 4, 2021
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