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 default -I and -L paths for OpenBSD #4649

Merged
merged 1 commit into from
Jan 5, 2025
Merged

Conversation

dspx-plcr
Copy link

@dspx-plcr dspx-plcr commented Jan 2, 2025

These can be added manually via environment variables when building, but it's probably sensible to have them as default on OpenBSD.

@zolk3ri
Copy link
Contributor

zolk3ri commented Jan 3, 2025

What issue does this change aim to address, considering that llvm-config is already responsible for providing the -I and -L paths? For example:

$ llvm-config --cxxflags --ldflags
-I/usr/include -std=c++17 -fno-exceptions -funwind-tables -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-L/usr/lib

Additionally, other platforms in this script rely solely on llvm-config for similar configurations. Hardcoding paths specifically for OpenBSD could lead to inconsistencies across platforms.

Could you clarify the rationale behind this change?

@laytan
Copy link
Collaborator

laytan commented Jan 3, 2025

Ditto, never heard anyone say it doesn't build with the current settings, and it shouldn't need it.

Are you perhaps confusing this build script with the actual Odin linking code?

@laytan laytan closed this Jan 3, 2025
@dspx-plcr
Copy link
Author

Hiya,

Sorry, I should have explained why I was suggesting this. The symptom I'm trying to solve is that odin does not build on OpenBSD 7.6.

[despereaux@polacre ~/build/Odin]$ git status
HEAD detached at origin/master
nothing to commit, working tree clean
[despereaux@polacre ~/build/Odin]$ git remote get-url origin
https://github.com/odin-lang/Odin.git
[despereaux@polacre ~/build/Odin]$ LLVM_CONFIG=/usr/local/llvm17/bin/llvm-config make release-native
./build_odin.sh release-native
+ clang++ src/main.cpp src/libtommath.cpp -Wno-switch -Wno-macro-redefined -Wno-unused-value -DODIN_VERSION_RAW="dev-2025-01" -DGIT_SHA="fdd3b46" -std=c++14 -I/usr/local/llvm17/include -std=c++17 -fno-exceptions -funwind-tables -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -L/usr/local/llvm17/lib -O3 -march=native -pthread -lm -lstdc++ -liconv -lLLVM -o odin
In file included from src/main.cpp:2:
In file included from src/common.cpp:68:
src/string.cpp:504:11: fatal error: 'iconv.h' file not found
        #include <iconv.h>
                 ^~~~~~~~~
1 error generated.
*** Error 1 in /home/despereaux/build/Odin (Makefile:19 'release-native')
[despereaux@polacre ~/build/Odin]$ uname -a
OpenBSD polacre 7.6 GENERIC.MP#338 amd64

(I have to use the OpenBSD llvm-17 package as 7.6 ships with llvm 16, which isn't supported by odin's build script)

The problem is that iconv is a third party package and hence is installed in the /usr/local prefix, whereas llvm is maintained as part of the base system, so gets special treatment. As a side note, even if llvm16 did work, this special treatment means that,

[despereaux@polacre ~/build/Odin]$ llvm-config --cxxflags --ldflags
-I/usr/include -std=c++17   -fno-exceptions -fno-rtti -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-L/usr/lib

so compiling odin would still fail.

Fundamentally, llvm-config tells you about how to compile and link against LLVM, not any other library, and iconv is some other library.

If you just add the -I path, then linking fails, so the -L path is also necessary.

@zolk3ri
Copy link
Contributor

zolk3ri commented Jan 4, 2025

How about using the pkg-config (or pkgconf) approach instead? It would dynamically detect the include and library paths for iconv, avoiding hardcoding paths.

Admittedly, I’m not too familiar with how Odin is configured and built, but this seems like a more flexible and maintainable solution.

(I have to use the OpenBSD llvm-17 package as 7.6 ships with llvm 16, which isn't supported by odin's build script)

Just to be sure; does it work with llvm-17 or do you still experience the issue regarding iconv?

@dspx-plcr
Copy link
Author

I agree that the pkg-config (or pkgconf) option is better semantically, but practically, the gnu libiconv package doesn't ship with a pkg config file, so it doesn't help. In terms of maintainability, I can't speak first hand for the history, but I took a look at the man pages from OpenBSD 2.8 in 2000, and /usr/local is the prefix for third party ports 🙂

I understand the aversion though, and ultimately it's the the Odin project's call as to if it's acceptable or not. Either way I'll be keeping the diff on my local branch because I find it useful.

Indeed the problem does persist with llvm-17, as the prefix paths for libiconv and llvm-17 are still different.

@laytan laytan reopened this Jan 5, 2025
@laytan laytan merged commit aa4bc10 into odin-lang:master Jan 5, 2025
7 checks passed
@zolk3ri
Copy link
Contributor

zolk3ri commented Jan 6, 2025

/usr/local is the prefix for third party ports

Yes, you are right, given that third-party software gets installed to /usr/local as stated in the OpenBSD documentation; this PR makes sense.

My apologies!

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