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

libINIReader depends on libinih, the order is important when these libraries are static libraries #3024

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

leleliu008
Copy link
Contributor

@leleliu008 leleliu008 commented Aug 9, 2024

close #2965

@kmilos
Copy link
Collaborator

kmilos commented Aug 10, 2024

Thanks. Have you maybe tried linking just w/ libINIReader?

@leleliu008
Copy link
Contributor Author

Thanks. Have you maybe tried linking just w/ libINIReader?

No. just linking libINIReader would arise undefiend symbol when libINIReader is libINIReader.a

@kmilos
Copy link
Collaborator

kmilos commented Aug 12, 2024

I meant just inih::inireader... That target should already imply the inih::libinih one, so we don't need both, no? Then the problem of ordering simply goes away...

@leleliu008
Copy link
Contributor Author

inih

I meant just inih::inireader... That target should already imply the inih::libinih one, so we don't need both, no? Then the problem of ordering simply goes away...

No. please read https://github.com/Exiv2/exiv2/blob/main/cmake/Findinih.cmake

@kmilos
Copy link
Collaborator

kmilos commented Aug 12, 2024

Ah, thanks for that reminder, I thought libinih shipped its own .cmake config files...

We might want to think about fixing Findinih.cmake then, but then there is always the question of what Conan does with its generated one...

@neheb
Copy link
Collaborator

neheb commented Aug 12, 2024

is this only relevant for cmake?

@leleliu008
Copy link
Contributor Author

leleliu008 commented Aug 12, 2024

https://github.com/benhoyt/inih use meson build system. It doesn't install cmake module file but install .pc files.

/Users/fpliu/.ppkg/installed/macos-14.4.1-arm64/inih
├── include
│   ├── INIReader.h
│   └── ini.h
└── lib
    ├── libINIReader.0.dylib
    ├── libINIReader.a
    ├── libINIReader.dylib -> libINIReader.0.dylib
    ├── libinih.0.dylib
    ├── libinih.a
    ├── libinih.dylib -> libinih.0.dylib
    └── pkgconfig
        ├── INIReader.pc
        └── inih.pc

4 directories, 10 files

@kmilos
Copy link
Collaborator

kmilos commented Aug 12, 2024

is this only relevant for cmake?

Yes. The .pc files are already correct: INIReader.pc already requires inih.pc...

@neheb
Copy link
Collaborator

neheb commented Aug 12, 2024

Right.

/me doesn't understand cmake files. Or any advantages over pkgconfig.

Copy link
Collaborator

@kmilos kmilos left a comment

Choose a reason for hiding this comment

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

Let's maybe merge this tweak for now, and handle Findinih.cmake later eventually. Thanks.

@kmilos kmilos merged commit 75c12fb into Exiv2:main Aug 16, 2024
57 of 58 checks passed
@kmilos
Copy link
Collaborator

kmilos commented Aug 16, 2024

@mergify backport 0.28.x

Copy link
Contributor

mergify bot commented Aug 16, 2024

backport 0.28.x

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake Configuration issues related with CMake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmake出错了
3 participants