-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: Deprecate is_<os> functions in favor of is<os> #22182
Conversation
base/deprecated.jl
Outdated
@deprecate $f() $newf() | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Five-liner (also seems simpler?):
@deprecate is_apple isapple
@deprecate is_bsd isbsd
@deprecate is_linux islinux
@deprecate is_unix isunix
@deprecate is_windows iswindows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize you could do it without explicitly providing the methods. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works because all methods of is_apple
etc. are deprecated, so it is not necessary to single out some of them :)
c50acf8
to
3238b36
Compare
Can't we replace them eg. by a single isos command? |
Probably, though that would take some design brainstorming to come up with something decently ergonomic. Personally I like the simplicity of just being able to say |
I do see that, but it's also a bit strange, since it doesn't say what is windows. |
I do agree with that, I just think of it like a less annoying way of saying We could ramp up the complexity and do a type-based approach, like |
3238b36
to
36dffc3
Compare
I'm not sure this is a net benefit. I think the goal of the underscore audit is to tend towards single word functions (possible compound), not to make them harder to read. (And I much prefer function_with_many_adjectives over |
Audit or not, with this I was more striving for consistency with other predicates, since we have 74 Base functions that begin with |
Why would that be impractical? We would just need one type for each of the currently supported functions. |
I hate the churn of renaming these again, but the original PR should have had a bit more naming discussion on it. These shouldn't have had underscores in them when they were exported. |
Are there scripts available for automatic conversion of these kinds of things? |
To update source code to the new version, I mean. |
@dpsanders would you like this abstract type OS end
struct Linux <: OS end
struct MacOS <: OS end
struct Windows <: OS end
isos(os::Type{<:OS}) = Sys.KERNEL === Symbol(os) so you can use julia> isos(Linux)
true
julia> isos(Windows)
false Actually I don't know what However, I see that this probably doesn't solve all issues, because these functions actually look at the kernel, rather than the system, and there are some systems (Apple, BSD, Windows) associated with different kernels. |
We'd end up needing a type for every OS under the sun. |
Could just be an enum. |
I suppose. I don't see that as being much more convenient than status quo though. |
Is that many type thing bad? Inheritance seems a fairly sane way of representing the relationship between different OSs. We could have a unix class with subtypes linux, mac and bsd etc. Is there a reason having a function for each is better than a type for each? |
I maintain that this PR is the path of least resistance, since it's a one-character change from status quo, and the current status quo came after a huge code churn that turned OS-specific macros into function calls. Moving to yet another approach will result in significantly more code churn for (IMHO) little benefit. If people are really interested in a type-based approach, I made a prototype in https://github.com/ararslan/OperatingSystems.jl. Feel free to play around with that and see if you like it. |
@dpsanders it's an enum way of thinking. i don't know a lot about the last status quo, but generally the coupling with macro and functions are due to other concerns, mainly maintaining readability during macro expansion. i am not so much affirmative telling this refactoring has to be done there and now. but i guess those kind of data concern will popup more and more frequently. untill one day we may have official guideline (to what i consider may be as the unique weakness of julia today) :) |
I don't see the difference with the current functions. We don't provide I'm not saying this necessarily The Right Approach, but any code churn is annoying for packages, so I think it's worth ensuring we won't break this again later. A one-char change is almost as annoying as moving to a different expression, as long as a systematic replacement can be applied. |
I've opened #22214 to discuss alternate approaches. If possible, I think we should keep the discussion in this PR limited to the approach implemented herein and move discussion of alternate systems to the linked issue. |
36dffc3
to
083773f
Compare
@stevengj made a good point in #22214 that even if we have an alternate system for representing operating systems, it's hard to beat the convenience of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. Personally I prefer underscore separated words but that ship has sailed.
Fine with me. I'm not totally convinced that |
083773f
to
2fb8db9
Compare
ffbfa04
to
8260c13
Compare
NEWS.md
Outdated
@@ -120,6 +120,10 @@ Deprecated or removed | |||
* The `corrected` positional argument to `cov` has been deprecated in favor of | |||
a keyword argument with the same name (#21709). | |||
|
|||
* The operation identification functions: `is_linux`, `is_bsd`, `is_apple`, `is_unix`, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
operating system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whops
base/osutils.jl
Outdated
@static | ||
|
||
Partially evaluates an expression at parse time. | ||
|
||
For example, `@static is_windows() ? foo : bar` will evaluate `is_windows()` and insert either `foo` or `bar` into the expression. | ||
For example, `@static iswindows() ? foo : bar` will evaluate `iswindows()` and insert either |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sys.iswindows
a70e6ae
to
d3ca6fa
Compare
base/c.jl
Outdated
@@ -4,6 +4,11 @@ | |||
|
|||
import Core.Intrinsics: cglobal, bitcast | |||
|
|||
# Equivalent to Sys.iswindows(), but that's not yet defined | |||
const _WINDOWS = let k = ccall(:jl_get_UNAME, Any, ()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of annoying to give this a name. I think we have control over jl_get_UNAME
on windows so it should only ever return a single value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we check it for :Windows
or :NT
in iswindows
then? Does that mean we could simplify that conditional as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it can take a symbol input, and partly backwards compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should only ever return a single value
Which value is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NT
Line 29 in 197ca1c
FLAGS += -DJL_BUILD_UNAME='"NT"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a necessary change? Giving it a name at least avoids having to do the ccall
several times, plus calling it Windows here is clearer than having to know that the Windows uname is NT. I don't care too much though, just seems unnecessary to change what's here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
giving it a name means it's there in Base for everyone to misuse. the ccall happens only at system image build time. otherwise could use a let to avoid occupying namespace in Base
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
d3ca6fa
to
81f7b32
Compare
base/c.jl
Outdated
@@ -49,7 +50,7 @@ Equivalent to the native `wchar_t` c-type ([`Int32`](@ref)). | |||
""" | |||
Cwchar_t | |||
|
|||
if !is_windows() | |||
if ccall(:jl_get_UNAME, Any, ()) !== :NT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this wrong? It seems to be causing a SIGABRT on Windows...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @tkelman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works locally? Maybe there's something strange about evaluating it eagerly, maybe (()->ccall(:jl_get_UNAME, Any, ()))()
would defer it to runtime or something? But then how did the const version that you had before work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have absolutely no idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the first ccall
works fine, it's just this second one that's getting upset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a bug to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we go with a workaround in the meantime? I'm not sure how to reproduce, much less diagnose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone with a workaround but added a note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I agree it's very strange that the const version works but without the const it does not. It sounds like it could be dangerous to go with the workaround unless the issue is understood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workaround is harmless, its necessity is just mysterious. I'm submitting an issue to track the underlying bug but in the meantime this should be fine.
e325593
to
7122b2e
Compare
Previous AppVeyor SIGABRT reported here: #22713 |
base/c.jl
Outdated
@@ -18,7 +24,8 @@ Equivalent to the native `char` c-type. | |||
""" | |||
Cchar | |||
|
|||
if is_windows() | |||
# The ccall is equivalent to Sys.iswindows(), but that's not defined yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment no longer applies. I'll remove that in a separate, CI skipped commit once the current CI run finishes, since removing the comment is a non-functional change and is not worth rerunning CI over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed my mind and just squashed it into the other commit so that AV gets a chance to run now that a fix for the libgit2-online test has been merged.
3864bc2
to
7114ddf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm if ci is clear
Both 32- and 64-bit Linux timed out. 😑 Tests passed on Windows and macOS (which in particular means |
31e223a
to
423cf04
Compare
This makes them consistent with the vast majority of Base predicates, which simply prefix with `is` rather than `is_`. It also moves to requiring the `Sys.` prefix, which helps explain the intent behind the function names.
423cf04
to
a1aad34
Compare
32-bit Linux is a timeout, previous run passed. Merging. |
This makes them consistent with the vast majority of Base predicates, which simply prefix with `is` rather than `is_`. It also moves to requiring the `Sys.` prefix, which helps explain the intent behind the function names.
This PR deprecates the OS predicate functions which contain underscores in favor of ones without underscores, thereby making them consistent with the vast majority of Base predicates, which simply prefix with
is
rather thanis_
. That is,is_unix
for example is nowisunix
.This is part of the underscore audit as mentioned in #20402.