RFC: Deprecate is_<os> functions in favor of is<os> #22182

ararslan · 2017-06-02T00:13:32Z

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 than is_. That is, is_unix for example is now isunix.

This is part of the underscore audit as mentioned in #20402.

fredrikekre · 2017-06-02T00:38:18Z

base/deprecated.jl

+        @deprecate $f() $newf()
+    end
+end
+


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

I didn't realize you could do it without explicitly providing the methods. Thanks.

It works because all methods of is_apple etc. are deprecated, so it is not necessary to single out some of them :)

dpsanders · 2017-06-02T00:52:55Z

Can't we replace them eg. by a single isos command?

ararslan · 2017-06-02T00:59:48Z

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 iswindows(), for example.

dpsanders · 2017-06-02T01:01:11Z

I do see that, but it's also a bit strange, since it doesn't say what is windows.

ararslan · 2017-06-02T01:17:35Z

I do agree with that, I just think of it like a less annoying way of saying iswindows(Sys.KERNEL) and that helps me feel better about it.

We could ramp up the complexity and do a type-based approach, like struct MacOS end and if @__OS__ == MacOS or if @__OS__ isa UnixBasedOS. Kind of fun, if impractical. 😛

vtjnash · 2017-06-02T02:55:50Z

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 fun(Val{true}, Val{'c'}) any day)

ararslan · 2017-06-02T02:59:39Z

Audit or not, with this I was more striving for consistency with other predicates, since we have 74 Base functions that begin with is and only 6 (5 of which are these) begin with is_. It seems unfortunate to keep this as basically the sole exception.

nalimilan · 2017-06-02T07:19:36Z

We could ramp up the complexity and do a type-based approach, like struct MacOS end and if @OS == MacOS or if @OS isa UnixBasedOS. Kind of fun, if impractical. 😛

Why would that be impractical? We would just need one type for each of the currently supported functions.

tkelman · 2017-06-02T08:04:51Z

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.

dpsanders · 2017-06-02T08:31:19Z

Are there scripts available for automatic conversion of these kinds of things?

dpsanders · 2017-06-02T08:31:36Z

To update source code to the new version, I mean.

giordano · 2017-06-02T14:32:49Z

@dpsanders would you like this isos?

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 Sys.KERNEL gives on other systems, I tried only on GNU/Linux.

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.

ararslan · 2017-06-02T18:43:38Z

Why would that be impractical?

We'd end up needing a type for every OS under the sun.

dpsanders · 2017-06-02T19:00:51Z

Could just be an enum.

ararslan · 2017-06-02T19:32:25Z

I suppose. I don't see that as being much more convenient than status quo though.

oscardssmith · 2017-06-02T20:00:01Z

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?

ararslan · 2017-06-02T21:27:50Z

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.

o314 · 2017-06-03T01:02:48Z

@dpsanders it's an enum way of thinking.
but the way @enum is currently implemented do not deserve well a language such as julia. it's too much tricky.
@giordano touches substantilly a very important point : how to map subtyping and union type cleanly.
we will have to adress more and more api over the internet and that will leads to json schema and lately bnf. this proposal is far more robust in this way.

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) :)

nalimilan · 2017-06-04T12:19:10Z

We'd end up needing a type for every OS under the sun.

I don't see the difference with the current functions. We don't provide isdragonfly, so why would we have to provide a DragonFly type? The type-based approach would allow us to return BSD for Dragonfly, just like we now have isbsd. Then if one day it turns out we need to distinguish Dragonfly, we'll be able to introduce a Dragonfly <: BSD type, and ensure backward compatibility.

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.

ararslan · 2017-06-04T23:35:23Z

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.

ararslan · 2017-06-14T02:17:49Z

@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 iswindows(). Having a concise representation for this is desirable as these checks can be quite common. We can always switch out the implementation of iswindows() to be Sys.OS <: Sys.Windows (or whatever) later without removing these functions, but in the meantime I think we should go ahead with this change.

omus

Changes look good. Personally I prefer underscore separated words but that ship has sailed.

nalimilan · 2017-06-14T07:52:02Z

Fine with me. I'm not totally convinced that iswindows is so much better than Sys.OS <: Sys.Windows, but if that's the general sentiment, then better remove the underscore now.

tkelman · 2017-06-29T20:16:45Z

NEWS.md

@@ -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


operating system

tkelman · 2017-06-29T20:17:58Z

base/osutils.jl

    @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


Sys.iswindows

tkelman · 2017-06-30T02:04:20Z

base/c.jl

@@ -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, ())


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

Why do we check it for :Windows or :NT in iswindows then? Does that mean we could simplify that conditional as well?

because it can take a symbol input, and partly backwards compatibility

it should only ever return a single value

Which value is it?

NT

julia/src/Makefile

Line 29 in 197ca1c

FLAGS += -DJL_BUILD_UNAME='"NT"'

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.

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

ararslan · 2017-07-01T05:07:45Z

base/c.jl

@@ -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


Is this wrong? It seems to be causing a SIGABRT on Windows...

cc @tkelman

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?

I have absolutely no idea.

Also the first ccall works fine, it's just this second one that's getting upset

this seems like a bug to me

Can we go with a workaround in the meantime? I'm not sure how to reproduce, much less diagnose.

I've gone with a workaround but added a note.

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.

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.

ararslan · 2017-07-08T19:52:31Z

Previous AppVeyor SIGABRT reported here: #22713

ararslan · 2017-07-08T23:51:12Z

base/c.jl

@@ -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


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.

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.

tkelman

lgtm if ci is clear

ararslan · 2017-07-09T19:18:05Z

Both 32- and 64-bit Linux timed out. 😑 Tests passed on Windows and macOS (which in particular means @static fixed the SIGABRT issue). Should I restart the Linux builds?

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.

ararslan · 2017-07-11T18:51:50Z

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.

JuliaLang/julia#22182](JuliaLang/julia#22182)

ararslan added the kind:deprecation This change introduces or involves a deprecation label Jun 2, 2017

ararslan requested a review from StefanKarpinski June 2, 2017 00:13

fredrikekre reviewed Jun 2, 2017

View reviewed changes

ararslan force-pushed the aa/os_underscores branch from c50acf8 to 3238b36 Compare June 2, 2017 00:43

ararslan force-pushed the aa/os_underscores branch from 3238b36 to 36dffc3 Compare June 2, 2017 02:13

ararslan mentioned this pull request Jun 4, 2017

Redefining how to identify operating systems #22214

Closed

ararslan force-pushed the aa/os_underscores branch from 36dffc3 to 083773f Compare June 13, 2017 05:07

omus approved these changes Jun 14, 2017

View reviewed changes

ararslan force-pushed the aa/os_underscores branch from 083773f to 2fb8db9 Compare June 16, 2017 18:16

ararslan force-pushed the aa/os_underscores branch 2 times, most recently from ffbfa04 to 8260c13 Compare June 29, 2017 20:15

tkelman reviewed Jun 29, 2017

View reviewed changes

ararslan force-pushed the aa/os_underscores branch 2 times, most recently from a70e6ae to d3ca6fa Compare June 30, 2017 01:38

tkelman reviewed Jun 30, 2017

View reviewed changes

ararslan force-pushed the aa/os_underscores branch from d3ca6fa to 81f7b32 Compare June 30, 2017 19:24

ararslan commented Jul 1, 2017

View reviewed changes

ararslan force-pushed the aa/os_underscores branch 2 times, most recently from e325593 to 7122b2e Compare July 8, 2017 19:30

ararslan commented Jul 8, 2017

View reviewed changes

ararslan force-pushed the aa/os_underscores branch 2 times, most recently from 3864bc2 to 7114ddf Compare July 9, 2017 06:47

tkelman approved these changes Jul 9, 2017

View reviewed changes

ararslan mentioned this pull request Jul 9, 2017

Multiple ccalls to jl_get_UNAME in base/c.jl causes AppVeyor to SIGABRT #22713

Closed

ararslan force-pushed the aa/os_underscores branch 2 times, most recently from 31e223a to 423cf04 Compare July 10, 2017 22:47

Deprecate is_<os> functions in favor of Sys.is<os>

a1aad34

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.

ararslan force-pushed the aa/os_underscores branch from 423cf04 to a1aad34 Compare July 11, 2017 02:05

ararslan merged commit 7cb31a8 into master Jul 11, 2017

ararslan deleted the aa/os_underscores branch July 11, 2017 18:52

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

danimad added a commit to danimad/LibCURL.jl that referenced this pull request Aug 11, 2018

Changed is_windows to Sys.iswindows because it was deprecated with [

556fce4

JuliaLang/julia#22182](JuliaLang/julia#22182)

danimad mentioned this pull request Aug 11, 2018

Changed is_windows to Sys.iswindows JuliaWeb/LibCURL.jl#43

Closed

jleugeri mentioned this pull request Aug 11, 2018

Change is_<os> to Sys.is<os> JuliaGizmos/Blink.jl#140

Closed

RFC: Deprecate is_<os> functions in favor of is<os> #22182

RFC: Deprecate is_<os> functions in favor of is<os> #22182

Conversation

ararslan commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpsanders commented Jun 2, 2017

ararslan commented Jun 2, 2017

dpsanders commented Jun 2, 2017

ararslan commented Jun 2, 2017

vtjnash commented Jun 2, 2017

ararslan commented Jun 2, 2017

nalimilan commented Jun 2, 2017

tkelman commented Jun 2, 2017

dpsanders commented Jun 2, 2017

dpsanders commented Jun 2, 2017

giordano commented Jun 2, 2017 • edited Loading

ararslan commented Jun 2, 2017

dpsanders commented Jun 2, 2017

ararslan commented Jun 2, 2017

oscardssmith commented Jun 2, 2017

ararslan commented Jun 2, 2017

o314 commented Jun 3, 2017

nalimilan commented Jun 4, 2017

ararslan commented Jun 4, 2017

ararslan commented Jun 14, 2017

omus left a comment

Choose a reason for hiding this comment

nalimilan commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Jul 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman left a comment

Choose a reason for hiding this comment

ararslan commented Jul 9, 2017

ararslan commented Jul 11, 2017

giordano commented Jun 2, 2017 •

edited

Loading

tkelman Jun 30, 2017 •

edited

Loading