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.

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

replace osutils macros with `@static` #16219

Merged
merged 3 commits into from May 21, 2016

Conversation

@vtjnash
Copy link
Member

commented May 5, 2016

implements #5892
closes #6674 and #4233
see discussion in those issues

@vtjnash vtjnash force-pushed the jn/static-if branch from 061fcf8 to f342c7d May 5, 2016

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented May 5, 2016

I like it. Question: what context are statically evaluated expressions evaluated in?

else
return esc(ex.args[2])
throw(ArgumentError("unknown operating system \"$os\""))

This comment has been minimized.

Copy link
@yuyichao

yuyichao May 5, 2016

Contributor

What's the exception garentee about @pure functions? If is_* can be @pure I believe a lot of the @static won't be necessary. (this also determines if we can make math functions like sin(::Float64) pure)

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 5, 2016

Author Member

I don't see any reason pure functions can't throw exceptions (preferably as long as they always throw the exception). fwiw, I don't actually enforce that the call in @static must be pure, but left that as a future exercise.

In a few places, I used @static mostly as a way of saving a few bytes in the sysimg. There's only a few contexts in which it makes a significant difference right now (ccall needs a constant name that is defined on the current system)

@stevengj

This comment has been minimized.

Copy link
Member

commented May 5, 2016

I dunno, still seems more convenient to be able to write @windows_only code... rather than @static if is_windows(); ...code...; end. Could we keep the OS convenience macros, even if they are implemented in terms of a more general @static construct?

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented May 5, 2016

Question: what context are statically evaluated expressions evaluated in?

Answer: current_module()

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented May 5, 2016

I dunno, still seems more convenient to be able to write @windows_only code... rather than @static if is_windows(); ...code...; end. Could we keep the OS convenience macros, even if they are implemented in terms of a more general @static construct?

how about @static is_windows() && code? I think we can easily add that syntax to the @static

otoh, I've found that the typical usage is to select between two (or more) implementations of something, in which case it can be replaced with a non-@static toplevel conditional

if is_windows()
   code
else
   more code
end
@tkelman

This comment has been minimized.

Copy link
Contributor

commented May 5, 2016

The @unix_only etc macros aren't too terrible, we could maybe hold off on getting rid of those for a later release - it's mainly the fake ternary macros that we should get rid of asap.

@tkelman

This comment has been minimized.

Copy link
Contributor

commented May 5, 2016

Also don't forget the NEWS.md mention.

push!(LOAD_CACHE_PATH,abspath(JULIA_HOME,"..","usr","lib","julia")) #TODO: fixme
push!(LOAD_PATH, abspath(JULIA_HOME, "..", "local", "share", "julia", "site", vers))
push!(LOAD_PATH, abspath(JULIA_HOME, "..", "share", "julia", "site", vers))
push!(LOAD_CACHE_PATH, abspath(JULIA_HOME, "..", "lib", "julia")) #TODO: fixme?

This comment has been minimized.

Copy link
@tkelman

tkelman May 5, 2016

Contributor

sneaky

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 5, 2016

Author Member

apparently not with you around 😜

This comment has been minimized.

Copy link
@tkelman

tkelman May 5, 2016

Contributor

this is one way of responding to aacf7d3#commitcomment-15940506

return Int32(0)
end
else
error("systemsleep undefined for this OS")

This comment has been minimized.

Copy link
@tkelman

tkelman May 5, 2016

Contributor

is this the only place where we error if we're neither unix nor windows?

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 5, 2016

Author Member

no, although sometimes we instead define the method as throwing an error instead of throwing it immediately

For example, `@static is_windows() ? foo : bar` will evaluate `is_windows()` and insert either `foo` or `bar` into the expression.
This is useful in cases where a construct would be invalid on other platforms,
such as a ccall to a non-existant functions.

This comment has been minimized.

Copy link
@tkelman

tkelman May 5, 2016

Contributor

existent

This comment has been minimized.

Copy link
@nalimilan

nalimilan May 8, 2016

Contributor

and "function". Also, the 92-char limit applies to docstrings too.

Predicate for testing if the OS is a derivative of BSD.
See documentation :ref:`Handling Operating System Variation <man-handling-operating-system-variation>`\ .
"""
is_bsd(os::Symbol = OS_NAME) = (os == :FreeBSD || os == :OpenBSD || os == :NetBSD || os == :Darwin)

This comment has been minimized.

Copy link
@tkelman

tkelman May 5, 2016

Contributor

guess dragonfly and solaris can wait for patches

if is_unix(); applies |= ("unix" in line.system); end
if is_apple(); applies |= ("osx" in line.system); end
if is_linux(); applies |= ("linux" in line.system); end
if is_bsd(); applies |= ("bsd" in line.system); end

This comment has been minimized.

Copy link
@tkelman

tkelman May 5, 2016

Contributor

should be added to the Requirements Specification docs

"""
is_apple([os])
Predicate for testing if the OS is a derivative of Apple Macintosh OS X or Darwin.

This comment has been minimized.

Copy link
@stevengj

stevengj May 5, 2016

Member

Probably is_macos? Do we want this to return true for iOS?

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 6, 2016

Author Member

Do we want this to return true for iOS?

I think yes. I believe iOS is a very similar kernel and user-space to the desktop platform.

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented May 6, 2016

OS_NAME was not changed AFAICT, so this doesn't fix #6674 yet.

@vtjnash vtjnash force-pushed the jn/static-if branch from f342c7d to 5c0882e May 13, 2016

OS_NAME
let UNAME = ccall(:jl_get_UNAME, Any, ())
# change some `uname -s` values to "friendly" names
# for internal usage

This comment has been minimized.

Copy link
@tkelman

tkelman May 13, 2016

Contributor

Why not advertise these as the public facing constants? uname, Darwin, kernel specifics etc are pretty obscure for a lot of users.

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 13, 2016

Author Member

because the kernel identifier is canonical and unambiguous. these are sort of just random substitutions. I'm more inclined to just drop this renaming and make Base.OSNAME === Sys.KERNEL. this does the renaming only because the exported names below use the "friendly" names.

This comment has been minimized.

Copy link
@tkelman

tkelman May 13, 2016

Contributor

What's canonical about WINNT? uname is a posix thing. The Julia function names and the Julia constant identifiers should be consistent, I don't see why shell scripting jargon matters.

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 13, 2016

Author Member

I think the correct name would be NT. I'll change our make file to use that instead and flow the changes through to here. WindowsNT, Windows_NT, WinNT, winnt, and WINNT all seem to be common variants though, so I had gone with our Makefile normalization initially

This comment has been minimized.

Copy link
@tkelman

tkelman May 13, 2016

Contributor

No one is going to write code testing against "NT." Formal kernel names are internal implementation details that don't matter so much at the Julia level - the userland OS, environment, and common names that people will want to test against and know what it means even if they've never written a shell script that called uname, do matter.

This comment has been minimized.

Copy link
@tkelman

tkelman May 16, 2016

Contributor

what would you call it?

A userspace posix compatibility layer. It doesn't implement a filesystem or other things that it just delegates to win32.

midipix would allow us to bypass the restrictions of the win32 wrapper and access the nt kernel directly.

I doubt this is really possible, unless you're microsoft and building code into the kernel (like the WSL subsystem).

If the goal of midipix is run Linux applications on Windows, WSL does a better job. midipix has failed in that regard, in that they're not measurably getting closer to anything we can use. The Linux binaries of Julia do actually run in WSL though (not perfectly, but the basics work). It doesn't make the win32 build of Julia any less necessary because it doesn't interoperate and can't run on older than Windows 10, but this is beside the point. The value of this variable is not actually the kernel, so it shouldn't be called that.

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 16, 2016

Author Member

It doesn't implement a filesystem or other things that it just delegates to win32.

Did I forget filesystem that in my list above? My bad. Cygwin also implements symlinks, filename mappings, and permissions using nt as the HAL. It also supports filesystems such as procfs and handles mounting in some cases such as folders.

No it wouldn't, midipix still goes through win32. Unless you're microsoft and building code into the kernel (like the WSL subsystem), there's no way to avoid that.

While the nt kernel interface is almost undocumented, sufficient information is available due to the need to write drivers against the nt side that it is possible to avoid it. Afaict from skimming the code in midipix, it skips the win32 layer and calls directly into nt for a number of syscalls. (I don't know whether they avoid linking to win32 entirely, or just in specific instances, although I suspect the former may be true).

because it doesn't interoperate

yes, this is exactly the fatal flaw in the whole WSL project. midipix may not have this shortcoming and thus is still more interesting (imo).

This comment has been minimized.

Copy link
@tkelman

tkelman May 16, 2016

Contributor

Only fatal if you would like win32 to cease to exist. Clearly Microsoft does not feel that way, and WSL will be more useful than midipix for some time to come. midipix is also GPL licensed at the moment which I consider to be a similarly fatal flaw for people who would like to create Windows applications using Julia.

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 16, 2016

Author Member

What is the point of your argument? This appears to be a validation that midipix would is a viable counterpart to win32 as a api to the nt kernel, while wsl is an independent, disconnected kernel. License may be a concern for actually being able to use it, but it's not relevant to a discussion of what it is called.

It sounds like WSL may provide yet another cross-compile environment that we will be expected to support, without providing any benefit to end users. Since it lies about it's uname, this will likely be made more difficult, so forgive me if I'm not quite thrilled about this. cygwin is at least truthful about its uname and allows coding against both the win32 and posix APIs to create hybrid apps.

This comment has been minimized.

Copy link
@tkelman

tkelman May 16, 2016

Contributor

We're off in the weeds, but mainly I don't think Sys.KERNEL should be called that.

The benefit of WSL to end-users is being able to do things like apt-get install openmpi-bin and have it work (without a VM or installing a Linux partition, which wouldn't interoperate either) without having to teach every single build system in the world what to do with a new uname value. It's disconnected from the win32 subsystem, but uses the same NT kernel - just not in a way that's exposed to the user, who never needs to worry about system calls. Supporting WSL from a Julia standpoint will just be a matter of running our Linux binaries there, unmodified, and reporting to Microsoft when things don't work.

@@ -82,8 +82,8 @@ run(pipeline(`echo hello world`, file))
rm(file)

# Stream Redirection
let
r = Channel(1)
if !is_windows() # WINNT reports operation not supported on socket (ENOTSUP) for this test

This comment has been minimized.

Copy link
@tkelman

tkelman May 13, 2016

Contributor

was it working before?

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 13, 2016

Author Member

no, it failed on Appveyor when I tried enabling it in the previous commit

@tkelman

This comment has been minimized.

Copy link
Contributor

commented May 15, 2016

The OS makefile variable isn't really the kernel - it's not accurate for WSL for example. Considering Julia's all in userland, I don't think we should be using that terminology. We could call it Sys.uname() but that has no meaning on Windows.

@vtjnash vtjnash force-pushed the jn/static-if branch from 5c0882e to 1f8eecb May 16, 2016

@tkelman

This comment has been minimized.

Copy link
Contributor

commented May 16, 2016

accepting bsd in REQUIRE still needs documentation.

how about not giving your Sys.KERNEL a name in Julia, and just using the ccall the few places it's needed?

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented May 16, 2016

how about not giving your Sys.KERNEL a name in Julia, and just using the ccall the few places it's needed?

I've considered making it a function instead of a constant (or populating the constant at runtime), but anyhow, I've already removed the constant from Base entirely now.

@tkelman

This comment has been minimized.

Copy link
Contributor

commented May 16, 2016

I've already removed the constant from Base entirely now.

Let's remove it from Sys entirely too. This doesn't need to be exposed and calling it KERNEL is incorrect.

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented May 16, 2016

Let's remove it from Sys entirely too. This doesn't need to be exposed and calling it KERNEL is incorrect.

How is it incorrect for a variable named KERNEL to contain the name of the kernel?

@tkelman

This comment has been minimized.

Copy link
Contributor

commented May 16, 2016

That's not what it contains, it contains what uname returned at compile time, munged by our makefile in the Windows case. This is not the name of the kernel. Even posix documents uname as returning the "operating system name."

@vtjnash

This comment has been minimized.

Copy link
Member Author

commented May 16, 2016

munged by our makefile in the Windows case.

Fixed.

Even posix documents uname as returning the "operating system name."

Posix usually treats the two as synonyms, although Linux doesn't (compare uname -s vs. uname -o). On some systems, these coincide so the point is moot. On Windows, the OS is Win32 while the Kernel is NT. On the Linux kernel, the OS is typically GNU/Linux.

src/gf.c Outdated
@@ -747,7 +747,7 @@ struct ambiguous_matches_env {
jl_array_t *shadowed;
int after;
};
const int eager_ambiguity_printing = 0;
const int eager_ambiguity_printing = 1;

This comment has been minimized.

Copy link
@tkelman

tkelman May 16, 2016

Contributor

unrelated

@vtjnash vtjnash force-pushed the jn/static-if branch from 8c2ae69 to 5581be9 May 16, 2016


.. function:: @linux
.. function:: windows_version()

This comment has been minimized.

Copy link
@tkelman

tkelman May 16, 2016

Contributor

is_windows needs a docstring

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 16, 2016

Author Member

isn't the genstdlib.jl supposed to warn about that? I hadn't checked since I assumed that's what all the "missing doc for" messages were intended to catch.

@vtjnash vtjnash force-pushed the jn/static-if branch from 5581be9 to 2899409 May 16, 2016

function round{T<:AbstractFloat, MR, MI}(z::Complex{T}, ::RoundingMode{MR}, ::RoundingMode{MI})
Complex((round(real(z), RoundingMode{MR}()),
round(imag(z), RoundingMode{MI}()))...)
end

This comment has been minimized.

Copy link
@tkelman

tkelman May 20, 2016

Contributor

grr fine, but this should at least have been a separate commit

@@ -1186,6 +1188,65 @@ end
isequal(x::Char, y::Integer) = false
isequal(x::Integer, y::Char) = false

#6674 and #4233
macro windows(qm,ex)
depwarn("`@windows` is deprecated, use `@static is_windows()` instead", Symbol("@windows"))

This comment has been minimized.

Copy link
@tkelman

tkelman May 20, 2016

Contributor

@static only works on an if

edit: sorry wrong line, the @windows_only warnings below should recommend the if form rather than the ternary form

function _cpu_summary(io::IO, cpu::Array{CPUinfo}, i, j)
if j-i < 9
header = true
for x = i:j
if header == false println(io) end
show(io,cpu[x],header,"#$(x-i+1) ")
header == false || println(io)

This comment has been minimized.

Copy link
@tkelman

tkelman May 20, 2016

Contributor

reversed sense?

tkelman added a commit to tkelman/julia that referenced this pull request May 21, 2016

@vtjnash vtjnash force-pushed the jn/static-if branch from 55b03f5 to 16088d1 May 21, 2016

@@ -3,6 +3,7 @@
using Core.Intrinsics: llvmcall

import Base: setindex!, getindex, unsafe_convert
import Base.Sys: Sys, WORD_SIZE

This comment has been minimized.

Copy link
@tkelman

tkelman May 21, 2016

Contributor

importing Sys from itself looks odd, why not ARCH if that's what's needed?

deprecate(:OS_NAME) # use Sys.KERNEL now

module Init_CPU_CORES
__init__() = eval(Base, :(@deprecate_binding CPU_CORES Sys.CPU_CORES))

This comment has been minimized.

Copy link
@tkelman

tkelman May 21, 2016

Contributor

I may have spoken too soon:

INFO: Recompiling stale cache file C:\cygwin64\tmp\jl_FE4C.tmp\FooBar.ji for module FooBar.
WARNING: eval from module Base to Main:
Expr(:macrocall, :@deprecate_binding, :CPU_CORES, Expr(:., :Sys, :CPU_CORES)::Any)::Any
  ** incremental compilation may be broken for this module **
@tkelman

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

Sorry about yet another NEWS.md conflict, #16435 was needed to fix the nightlies. And #16308 means that tkelman@0e81c84 will be necessary to get through bootstrap.

vtjnash and others added some commits May 5, 2016

implement `@static` macro for replacing osutils macros
implements #5892
closes #6674 and #4233

Sys.KERNEL now replaces OS_NAME and unambiguously returns
the name of the kernel reported by uname for the build system configuration

@vtjnash vtjnash force-pushed the jn/static-if branch from 16088d1 to 6154c85 May 21, 2016

@tkelman tkelman merged commit 7430e83 into master May 21, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tkelman tkelman deleted the jn/static-if branch May 21, 2016

finally
systemerror(:fchdir, ccall(:fchdir,Int32,(Int32,),fd) != 0)
systemerror(:close, ccall(:close,Int32,(Int32,),fd) != 0)
if is_windows()

This comment has been minimized.

Copy link
@joehuchette

joehuchette May 22, 2016

Member

is there a @static missing here?

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 22, 2016

Author Member

no, statically evaluating a top level static expression is redundant

tkelman added a commit to JuliaLang/Compat.jl that referenced this pull request May 23, 2016

@omus

This comment has been minimized.

Copy link
Member

commented May 25, 2016

Am I correct in that the removal of the @*_only macros means you can no longer mark a function as OS specific without the use of an if?

# old
@osx_only func() = 1

# new
if is_apple()
    func() = 1
end
"""
@static
Partially evaluates an expression at parse time.

This comment has been minimized.

Copy link
@stevengj

stevengj May 25, 2016

Member

This is pretty terse. Maybe:

Given a conditional expression like `@static cond ? yes : no` or `@static if cond; yes; else; no`, evaluates the
condition `cond` at parse time (via `eval`, i.e. in global scope for the current module) and produces either the `yes` or `no` expression.

For example, ...
@stevengj

This comment has been minimized.

Copy link
Member

commented May 25, 2016

@omus, the analogue of @osx_only is @static if is_apple(); func() = ...; end

@omus

This comment has been minimized.

Copy link
Member

commented May 25, 2016

@stevengj thanks, that's what I thought. It was nice being able to use the macros on long form functions:

@osx_only function func()
    ...
end

Oh well

kmsquire added a commit that referenced this pull request May 30, 2016

Undeprecate {os}_only macros
* These were convenient and less noisy than the current
  {@}static if is_{os} counterpart introduced in #16219
* {@}osx_only is now {@}apple_only, to match the change in #16219
* added {@}bsd_only

rfourquet added a commit to rfourquet/julia that referenced this pull request Jun 11, 2016

rfourquet added a commit to rfourquet/julia that referenced this pull request Jun 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.