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

add additional environment variables and add libraries/flags to linki… #636

Closed
wants to merge 1 commit into from

Conversation

jcreinhold
Copy link

@jcreinhold jcreinhold commented Mar 13, 2023

RFC

This PR addresses the problems raised in #635

For non-standard build setups, this adds OWL_LDFLAGS, OWL_CPPFLAGS, and OWL_LDLIBS which correspond to the usual gcc flags.

I personally don't think these are excessive, but we could change this to only add OWL_LDFLAGS (or, e.g., OWL_LIBRARY_FLAGS) which would be sufficient if this PR adds too many new environment variables. The current implementation in this PR, however, allows the user to specify flags like they would if they were directly using gcc. Even though the flags are eventually combined into c_flags and c_library_flags, this allows users to not cram everything into something labeled OWL_CFLAGS and (the minimally required for this to build) OWL_LDFLAGS.

This also adds the generated flags to the OpenBLAS linking tests with c_test which were previously failing for the reasons mentioned in #635. This change is required for non-standard builds; the linking test will fail simply because it's lacking the correct flags—even though owl would build with OWL_CFLAGS and something akin to OWL_LDFLAGS.

…ng tests to fix build issues in non-standard installs
@jcreinhold
Copy link
Author

FWIW, this solves many of the install problems (really linking problems) that occur with this library; albeit, this places the responsibility on the user to correctly configure the library. However, we're implicitly relying on them doing this by asking them to ln -s certain libraries to certain locations so they can be found by the compiler.

For example, I encountered problems linking libquadmath.dylib on my Macbook when I was installing the library. I am using a Apple M1 processor, so I incorporated the modifications to owl_core_utils.c and owl_macros.h listed in this PR—keeping everything else as in this current PR.

Instead of doing ad hoc ln -s or other work arounds that are particularly fragile, I was able to add OWL_LDFLAGS="-L/Users/jcreinhold/homebrew/Cellar/gcc/12.2.0/lib/gcc/current" and OWL_LDLIBS="-lquadmath" and successfully installed owl (building from source).

I was also able to use this PR (and @mseri aarch64 fix) to build this package at work on a very painful build system.

While it's a bit of a burden for the user to learn about how to configure this, I could add in some tutorial material, and perhaps an install FAQ based on the install issues, and the future install problems can be reduced to figuring out the right combination of flags to pass via these proposed environment variables.

@nilsbecker
Copy link
Contributor

nilsbecker commented Oct 26, 2023

I would very much welcome such a PR. I am used to setting linking and C flag variables for installing lacaml and gsl, and it works well there to use homebrew installed libs after reading some basic instructions in the respective READMEs.
(Currently, not sure how to get owl running on my m1 mac. Can I opam pin some fork of owl to get this PR + the relevant bits of the arm64 PR?)

mseri added a commit to mseri/owl that referenced this pull request Oct 27, 2023
See owlbarn#636

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
mseri added a commit to mseri/owl that referenced this pull request Oct 27, 2023
See owlbarn#636

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
@mseri
Copy link
Member

mseri commented Oct 27, 2023

@nilsbecker I included the change both in my arm64 and arm64-bak (which includes also changes from the open PRs) branches if you want to test it out

@nilsbecker
Copy link
Contributor

i just tied #625 (comment) and that worked for me -- i guess it's using your updated arm64 branch yes?

@mseri
Copy link
Member

mseri commented Oct 27, 2023

Yes

@nilsbecker
Copy link
Contributor

nilsbecker commented Oct 27, 2023

On another M1 mac I tried the same but keep getting a linker error that says ld: library 'lapacke' not found. any hints? brew install lapack did not help so far. It's confusing as I'm not aware of relevant differences between the two computers.

one more try:
PKG_CONFIG_PATH="/opt/homebrew/opt/lapack/lib/pkgconfig:/opt/homebrew/opt/openblas/lib/pkgconfig" opam install owl
fails with the same error, although pkg-config can find version 3.11 of lapacke with this path...

@oemmerson
Copy link

oemmerson commented Oct 28, 2023

@mseri How come the changes in your fork to support the M1 haven't been merged?

Thanks for the work to fix it BTW.

@mseri
Copy link
Member

mseri commented Oct 31, 2023

On another M1 mac I tried the same but keep getting a linker error that says ld: library 'lapacke' not found. any hints? brew install lapack did not help so far. It's confusing as I'm not aware of relevant differences between the two computers.

I don't know. You can check if OWL_DISABLE_LAPACKE_LINKING_FLAG=1 fixes it

@nilsbecker
Copy link
Contributor

OWL_DISABLE_LAPACKE_LINKING_FLAG=1

indeed that did allow it to install! the working command line was:
PKG_CONFIG_PATH="/opt/homebrew/opt/openblas/lib/pkgconfig" OWL_DISABLE_LAPACKE_LINKING_FLAG=1 opam install owl.1.1.0

i.e. without adding a path to the separately installed lapack library. no idea why it was necessary on this machine and not the other one.

@nilsbecker
Copy link
Contributor

ok, while disabling the lapacke linking did allow owl to get installed, it then does not work properly in an application. i get loads of linking errors saying that some lapacke symbols are not found.

adding the brew-installed lapack to the PKG_CONFIG_PATH when installing owl does not work either. so still no luck overall

@mseri
Copy link
Member

mseri commented Nov 1, 2023

I don't know, indeed OWL_DISABLE_LAPACKE_LINKING_FLAG=1 should not exist imho. If pkg-config has reasons to link lapacke it means it should be linked, but it was worth a try. Not sure what happens, do you have a mix of x86 and arm pagackes installed via homebrew? That can mess it up for example

@nilsbecker
Copy link
Contributor

no that should not be the problem. i did a rebuild of the homebrew installation from an exported package list.

i just noticed that i have an env variable set:
OWL_MANUAL_LDFLAGS=-L/opt/homebrew/opt/gfortran/lib/gcc/current -L/opt/homebrew/opt/openblas/lib
which i define in my bash_profile. i don't remember where this came from (shame on me). is this something recognized by owl? should i unset it?

also my default PKG_CONFIG_PATH=/opt/homebrew/opt/openblas/lib/pkgconfig:/opt/X11/lib/pkgconfig:/usr/X11/lib/pkgconfig...

@nilsbecker
Copy link
Contributor

i think the manual linking flags were from a previous workaround for installing owl. i just retried with LDFLAGS=$OWL_MANUAL_LDFLAGS opam install owl.1.1.0 but that did not remove the lapacke linking error, alas.

@nilsbecker
Copy link
Contributor

Ok, I've got it now. Mea culpa. For a reason I cannot fathom, my full opam switch was an x86_64 binary without me knowing it. It silently ran under Rosetta2 but when trying to link arm64-only libraries, that did not work anymore somehow. So @mseri 's initial hunch was correct: some x86 cruft was still around: all of the switch!

I was really convinced that this was a new install of ocaml via opam and not an old file tree transferred from a backup, but now i'm not so sure anymore.

Bottom line: the intallation instructions referenced above, pinning @mseri 's branch do work for me now.

jzstark pushed a commit that referenced this pull request May 1, 2024
* Progress towards ARM64 support

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* Re-introduce Ofast flag

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* unit_signal: fix test

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* Update architecture detection

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* ocamlformat

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* fixup! Update architecture detection

* Improve architecture and os detection

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* config: cleanup unused open

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* reintroduce -mfpmath=sse on x86_64

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* owl_macros.h: remove empty if

* owl/configure: remove devmode cflags

They break the compilation with some c compilers. See #609

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* owl_core_utils: Fix empty elif

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* Cleanup arch detection code

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* configure: update to support new lapacke packages

Debian now ships lapacke separately, requiring new set of flags for the linking to work.
This uses pkg-config to improve the detection of these flags.

Furthermore this commit adds a workaround to solve the issue with libgcc11_s not found,
which will not be addressed upstream in homebrew for the time being.

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* ocamlformat the code

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* Workaround configurator problem with windows

And remove march=native from arm, since it breaks on a number of arm systems

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* configurator: make more robust wrt not officially supported platforms (e.g. bsds)

* Update src/owl/config/configure.ml

Co-authored-by: Thomas Gazagnaire <thomas@gazagnaire.org>

* configurator: drop unnecessary dependency on stdio

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* add @jcreinhold patch for nonstandard setups

See #636

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

* Update src/aeos/config/configure.ml

* Update src/owl/config/configure.ml

---------

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
Co-authored-by: Thomas Gazagnaire <thomas@gazagnaire.org>
@jzstark
Copy link
Collaborator

jzstark commented May 13, 2024

Close for now since the configuration has been updated.

@jzstark jzstark closed this May 13, 2024
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.

None yet

5 participants