Skip to content

add -lrt on Solaris 10, required for nanosleep #999

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 6, 2021
Merged

add -lrt on Solaris 10, required for nanosleep #999

merged 1 commit into from
Aug 6, 2021

Conversation

pekdon
Copy link
Contributor

@pekdon pekdon commented Jun 2, 2021

To get access to nanosleep on Solaris 10, link against rt is required.

Cyan4973
Cyan4973 reviewed Jun 2, 2021
View reviewed changes
programs/Makefile

include ../Makefile.inc

ifeq ($(TARGET_OS)$(shell uname -r),SunOS5.10)
Copy link
Member

@Cyan4973 Cyan4973 Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What's the purpose of listing both TARGET_OS and $(shell uname -r)

since TARGET_OS is already defined as TARGET_OS ?= $(shell uname) ?

edit: ok, so TARGET_OS is the name, while -r is for the version.
Hence, it makes this patch very specific, to SunOS5.10 only.

Any reason why it has to be specific to this version only (as opposed to SunOS in general) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lz4 builds fine on Solaris 11 without it so I thought it was unnecessary to add it to all Solaris versions. I don't have access to any < 10 machines for testing either.

@Cyan4973
Copy link
Member

I'm a bit torn on this one.
On one hand, it's a simple change, well localized.
On the other hand, it's so specific, targeting a single version of a single (non-mainstream) OS, that I'm wondering if it should be part of general upstream, or part of a downstream patch dedicated to this platform only.

@servusdei2018
Copy link
Contributor

I'm a bit torn on this one.
On one hand, it's a simple change, well localized.
On the other hand, it's so specific, targeting a single version of a single (non-mainstream) OS, that I'm wondering if it should be part of general upstream, or part of a downstream patch dedicated to this platform only.

I propose a solution in the form of a solaris.mk file with Solaris-specific directives, and to include that in Makefile. Inside solaris.mk would be the business logic of all Solaris-specific configuration. This pattern would be a good architecture in the future, in the event that another OS needs some specific configuration or tweaking, and splits things into a neat hierarchical division.

Possibly, such OS-specific configurations could also be transferred to a new directory called make or something like that, in apprehension of such files cluttering the root directory.

@Cyan4973
Copy link
Member

Cyan4973 commented Aug 6, 2021

It looks like a good pattern @servusdei2018 .

It will require a bit of time to setup properly. Nothing too difficult, but time is scarce.
In the meantime, I'll merge this PR, on the ground that the logic is well concentrated in a single location.

@Cyan4973 Cyan4973 merged commit aeac622 into lz4:dev Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cyan4973 Cyan4973 Cyan4973 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pekdon @Cyan4973 @servusdei2018