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

Compilation issue. Do not provide vendor directory nor header #34

Open
ccoulombe opened this issue Oct 27, 2022 · 9 comments
Open

Compilation issue. Do not provide vendor directory nor header #34

ccoulombe opened this issue Oct 27, 2022 · 9 comments

Comments

@ccoulombe
Copy link
Contributor

When compiling, such vendored files are conflicting. It is best to ask for the dependencies such as boost to be fulfilled on the system where the software is built.
For instance, when including vendor/pthread.h it simply do not build.

error: #error "Never use <bits/endian.h> directly; include <endian.h> instead."
    4 | # error "Never use <bits/endian.h> directly; include <endian.h> instead."

Also, with Cmake it is easy to ask and find boost : find_package(boost REQUIRED)

Solution: in order to build, remove this folder and its inclusion in CMakeLists.txt.

@ccoulombe ccoulombe changed the title Do not provide vendor directory nor header Compilation issue. Do not provide vendor directory nor header Oct 27, 2022
@ccoulombe
Copy link
Contributor Author

Ah, it was correct in the latest tag (1.2.2) but was introduced in #33

@johnlees
Copy link
Member

How are you compiling to get this error?

We had to introduce the vendor approach to work with pypi and some other packages, but I could add a compile arg to cmake to go back to the old approach in v1.2.2 if the user wants

@ccoulombe
Copy link
Contributor Author

ccoulombe commented Oct 28, 2022

With python setup.py build on centos and gcc 9.3.0. (Conda is not an option.)

Can you detail why it was needed to had a vendor directory with some header files?

Dependencies and necessary header should all be provided by the system on which one install the software.
There's many packages on PyPI that requires Boost and do not distribute any of it, but ask for it.

@johnlees
Copy link
Member

@richfitz do you have any suggestions/opinions on this issue? I believe this was actually added for cases where conda isn't an option

Dependencies and necessary header should all be provided by the system on which one install the software.
There's many packages on PyPI that requires Boost and do not distribute any of it, but ask for it.

In principle I think agree, but in practise I just want to make it as pain free as possible to install the software in as many situations as possible 🙂

Whatever the case, I can always change the CMake file to allow the vendor directory to be ignored

@richfitz
Copy link
Contributor

My opinions are mostly variations on the theme that python packaging is irreparably broken.

An option to make the vendored version skippable, or in the presence of a reproducible example that triggers the conflict it could of course be worked around. Without that, that seems like something someone could do a PR against, should it bother them sufficiently.

@ccoulombe
Copy link
Contributor Author

ccoulombe commented Oct 28, 2022

What are the reasons/issues that lead to a vendor directory in the first place?

Providing a hack to avoid a bodge is not an alternative in my opinion :)

In principle I think agree, but in practise I just want to make it as pain free as possible to install the software in as many situations as possible slightly_smiling_face

Indeed!! Then asking for the dep to be fulfilled on the target system is the easiest for everyone I believe.

I'll push my PR with the patch I made for the systems installation.

@richfitz
Copy link
Contributor

It didn't work for us, with workflows we have. Currently it does. I am not incredibly inclined spend time explaining why we made choices that work for us by someone on the internet who the only interactions I have with are them couching their opinions as if they were facts. I would not describe it as a hack, though it's sad it does not work for you under circumstances that we still know nothing about. John may still arrange to make a change to make this configurable in the cmakefile if he gets time. It's an interesting approach you take and I hope you fare better with it on other open source projects you interact with.

to be fulfilled on the target system is the easiest for everyone I believe

You believe wrong, I am afraid.

@ccoulombe
Copy link
Contributor Author

If you read a few comments above you'll have the necessary context. But it still does not explain the issue... the issue that still reside is the one created by a vendor directory that is conflicting. I am here only to help fix it...

It is sad to be judged instead of being collaborative... I've spent time to fix/patch all software related to PopPUNK and wanted to share my experience and fixes, so more people can use it, more easily and without issues.

@johnlees
Copy link
Member

I'm happy to add an option which ignores the vendor directory and uses the system boost, but for one of our install set ups the vendor directory seems to be the best option for now so I don't want to totally remove it

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 a pull request may close this issue.

3 participants