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

Fix build on x86_64 #141

Merged
merged 1 commit into from
Jul 25, 2023
Merged

Fix build on x86_64 #141

merged 1 commit into from
Jul 25, 2023

Conversation

JakubOnderka
Copy link
Contributor

On same platform, library file is compiled into lib64 directory. Rust compiler expects that library will be in lib directory. This fixes this by searing library both in lib and lib64 directory.

On same platform, library file is compiled into `lib64` directory. Rust compiler expects that library will be in `lib` directory. This fixes this by searing library both in `lib` and `lib64` directory.
@Byron
Copy link
Member

Byron commented Jul 24, 2023

Thanks for the fix!

Could you explain more about the origin of the issue that is fixed here? I'd like to understand if that was an issue that was recently introduced, or if it is a zero-day that now just happens to surface. Maybe it's a different platform that's causing this that we might want to try to add to CI?

Thanks for elaborating.

@Byron Byron added the question There is a question to be answered label Jul 24, 2023
@JakubOnderka
Copy link
Contributor Author

JakubOnderka commented Jul 24, 2023

I am actually not sure, but there was changed in cmake scripts in zlib-ng to 2.1. And the default cmake detection is a mess :D
So I think if we want to test it in CI, we have to also test against different distribution than Ubuntu or Debian.

From cmake source code:

# Override this default 'lib' with 'lib64' iff:
#  - we are on Linux system but NOT cross-compiling 
#  - we are NOT on debian
#  - we are NOT building for conda
#  - we are on a 64 bits system

@Byron Byron merged commit d013542 into rust-lang:main Jul 25, 2023
@Byron
Copy link
Member

Byron commented Jul 25, 2023

Thanks a lot!

As CI is working, this means that having multiple search paths doesn't confuse rustc, so nothing should be able to break with this change. I also added the context you kindly provided to the merge commit for good measure.

Byron added a commit that referenced this pull request Jul 25, 2023
Additional context from #141 (comment)

I am actually not sure, but there was changed in cmake scripts in zlib-ng to 2.1. And the default cmake detection is a mess :D
So I think if we want to test it in CI, we have to also test against different distribution than Ubuntu or Debian.

From cmake source code:

```
 # Override this default 'lib' with 'lib64' iff:
 #  - we are on Linux system but NOT cross-compiling
 #  - we are NOT on debian
 #  - we are NOT building for conda
 #  - we are on a 64 bits system
```

Co-Authored-By: Jakub Onderka <[email protected]>
@Byron Byron removed the question There is a question to be answered label Jul 25, 2023
@torokati44
Copy link

Is there a plan to make a quick hotfix release with this change soon?

@Byron
Copy link
Member

Byron commented Jul 25, 2023

Right! Even though the breakage doesn't reproduce yet, it does exist and was introduced with the CMake upgrades in the more recent version of zlib-ng.

Anyway, here is the new release.

Unfortunately it doesn't seem super trivial to change the linux distribution with cross, but maybe @JakubOnderka or @torokati44 can provide information on how to get such a previously failing distribution to run under GitHub actions CI. This way, regression tests could be added for this, increasing coverage.

@torokati44
Copy link

torokati44 commented Jul 25, 2023

Actually it was a fellow contributor who had an error related to zlib.h when running clippy on Windows, after updating to the (previous) latest release; and linked to this issue. I just butted in here with my comment above to hopefully help things move along for him. 😶
It seems his issue is still not fixed, he might report it soon.

EDIT: Yep, there we go: #143

@Byron
Copy link
Member

Byron commented Jul 25, 2023

It's my first time to maintain a crate with 30m total downloads, and with the amount of users and platforms it's close to impossible to cover all bases. Thus, when PRs appear that fix something, I will try harder to be sure everything there is to know about it is disclosed before taking an action. Particularly, CI must be able to reproduce an issue and I don't think it's wise to proceed like I did thus far.

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