Skip to content

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

Merged
Cyan4973 merged 1 commit intolz4:devfrom
pekdon:solaris-10
Aug 6, 2021
Merged

add -lrt on Solaris 10, required for nanosleep#999
Cyan4973 merged 1 commit intolz4:devfrom
pekdon:solaris-10

Conversation

@pekdon
Copy link
Copy Markdown
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
Comment thread programs/Makefile

include ../Makefile.inc

ifeq ($(TARGET_OS)$(shell uname -r),SunOS5.10)
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@pekdon pekdon Jun 3, 2021

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
Copy Markdown
Member

Cyan4973 commented Jun 25, 2021

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
Copy Markdown
Contributor

servusdei2018 commented Aug 6, 2021

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
Copy Markdown
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