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.

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

Add Sys.is* for all recognized kernels #30249

Merged
merged 1 commit into from
Dec 5, 2018
Merged

Add Sys.is* for all recognized kernels #30249

merged 1 commit into from
Dec 5, 2018

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Dec 3, 2018 •
edited
Loading

I've found myself wanting Sys.isfreebsd while writing some FreeBSD-specific package code in my spare time. Per Elliot's suggestion (see below), this adds predicates for all kernels.

@ararslan ararslan added system:freebsd Affects only FreeBSD minor change Marginal behavior change acceptable for a minor release labels Dec 3, 2018
@ararslan ararslan requested a review from staticfloat December 3, 2018 20:26
@ViralBShah
Copy link
Member

Should it be isbsd to be a bit more general?

@musm
Copy link
Contributor

musm commented Dec 3, 2018

isbsd already exists,
see #22794 , where it seemed that most people did not favor adding this function

@ararslan
Copy link
Member Author

ararslan commented Dec 3, 2018

It was mostly me that didn't find it necessary at the time, but I've come around on it (hence the PR).

@staticfloat
Copy link
Member

I kind of feel we should just jump the shark and just provide a convenience function for every kernel we officially recognize (e.g. OpenBSD, NetBSD, etc....). It's not like it's a significant effort for us to support these single-line methods, and since it's so easy the typical argument of "don't add it until we need it" feels less strong somehow.

@ararslan
Copy link
Member Author

ararslan commented Dec 3, 2018

I agree, I like that approach. (Though who knows whether we'll even be able to support OpenBSD; they need a heinous number of their own patches just to build LLVM...)

@staticfloat
Copy link
Member

I like to think that we, as a community, error on the side of optimism, rather than pessimism.

@ararslan ararslan changed the title Add Sys.isfreebsd Add Sys.is* for all recognized kernels Dec 3, 2018
@ararslan ararslan removed the system:freebsd Affects only FreeBSD label Dec 3, 2018
fredrikekre
fredrikekre reviewed Dec 3, 2018
View reviewed changes
base/sysinfo.jl Outdated Show resolved Hide resolved
@ararslan ararslan added the needs news A NEWS entry is required for this change label Dec 3, 2018
@ararslan ararslan removed the needs news A NEWS entry is required for this change label Dec 3, 2018
@ararslan ararslan force-pushed the aa/sys-isfreebsd branch 3 times, most recently from 1e6a661 to a22e2a9 Compare December 4, 2018 16:48
@ararslan
Add Sys.is* for all recognized kernels
6b0ba40 
We have `Sys.is*` for a subset of supported platforms, but not for all
recognized kernels, e.g. FreeBSD, OpenBSD, etc. `Sys.isbsd` isn't
specific enough in some cases, and `Sys.KERNEL === x` is inconsistent
with other systems.
@staticfloat staticfloat merged commit ff0acfe into master Dec 5, 2018
@ararslan ararslan deleted the aa/sys-isfreebsd branch December 5, 2018 20:40
@giordano giordano mentioned this pull request Sep 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredrikekre fredrikekre fredrikekre left review comments

@staticfloat staticfloat staticfloat left review comments

Assignees
No one assigned
Labels
minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ararslan @ViralBShah @musm @staticfloat @fredrikekre