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 building on Solaris 10 x86_64 GCC 4.9.2 #3861

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

likema
Copy link
Contributor

@likema likema commented Jan 13, 2024

Prevent building x86_64 ASM from Solaris x86_64

@Cyan4973 Cyan4973 self-assigned this Jan 13, 2024
@Cyan4973
Copy link
Contributor

This PR conflicts with an old patch that was recently merged.
It should be trivially fixable though.

Prevent building x86_64 ASM from Solaris x86_64
@likema
Copy link
Contributor Author

likema commented Jan 13, 2024

This PR conflicts with an old patch that was recently merged. It should be trivially fixable though.

Fixed

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jan 29, 2024

I finally found the time to create a Solaris test for Github Actions.
It can be seen here : https://github.com/facebook/zstd/actions/runs/7690458663/job/20954282520?pr=3885

The test includes running cmake, both build and install stages.
I was expecting it to fail, considering the description of this PR, which is accepted but not merged, hence not yet applied to #3885.
But apparently, it's not necessary : the test just runs completely fine without it.

Among the potential differences that could explain this surprising outcome:
this PR mentions an issue with Solaris 10 and gcc 4.9.2,
while #3885 runs a Solaris VM version 11.4, shipping gcc 5.5.0.
So maybe something changed between versions 10 and 11 (gcc version notably).

Conversely, it also means that this patch might be a bit too strong:
if I read it correctly, it removes assembly support for any version of SunOS.
But, since 11.4 works fine, it could miss some performance due to the removal of this support.

Therefore, it seems a more adjusted solution would be to limit removal of asm support for Solaris version 10 and below.

@likema
Copy link
Contributor Author

likema commented Jan 29, 2024

You are right.

Checking if x86_64 ASM source can be compiled in cmake may be a better solution.

I will try to do it later

Copy link
Contributor

@Cyan4973 Cyan4973 left a comment

Choose a reason for hiding this comment

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

Find a different way for this constraint (no-asm) to be more limited,
since Solaris 11.4 (at least) doesn't seem to need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants