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

Add other Julia compilation flags #62

Merged
merged 1 commit into from
May 12, 2018

Conversation

lucatrv
Copy link
Collaborator

@lucatrv lucatrv commented May 10, 2018

No description provided.

@lucatrv
Copy link
Collaborator Author

lucatrv commented May 10, 2018

@SimonDanisch @NHDaly
this is a quite extensive PR, please review it, if you agree I will finalize and merge it.

@NHDaly
Copy link
Member

NHDaly commented May 10, 2018

I'm interested in the use case for disabling/enabling --precompiled and --compilecache. Is there a reason that a user of this package would want that? If so, could we maybe also explain that a bit in the docstring?

arg_type = String
metavar = "{yes|no}"
range_tester = (x -> x ∈ ("yes", "no"))
help = "load ~/.juliarc.jl"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the arg name, "--startup-file", I would assume that this flag allows me to set a custom startup file. Is that desirable? I would either consider changing the behavior to what I just described, or changing the name of the variable to something like --use-startupfile?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained below, juliac provides same compilation flags as julia (in future julia-compile). See julia -h.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay i'm convinced. :) thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry i misunderstood that what you were doing in this PR is just copying forward all of the julia flags! Makes sense.)

)
# TODO: these may just be temporary workarounds, see: https://github.com/JuliaLang/PackageCompiler.jl/issues/47
Copy link
Member

@NHDaly NHDaly May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"these" is unprecise. How about:

#  TODO: precompiled and compilecache may be removed, pending: https://github.com/JuliaLang/PackageCompiler.jl/issues/47

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll change that.

)
# TODO: these may just be temporary workarounds, see: https://github.com/JuliaLang/PackageCompiler.jl/issues/47
precompiled == nothing && cpu_target == nothing || (precompiled = "no")
compilecache == nothing && (compilecache = "no")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, i could be misunderstanding, but the first one doesn't feel correct to me.
If the user passes precompiled = "yes", won't this set precompiled to "no"?

I think maybe you want parentheses around the second two phrases?

 precompiled == nothing && (cpu_target == nothing || (precompiled = "no"))

But honestly that's still hard to parse. perhaps this can simply be an if-statement? I think this is what you meant?:

if (precompiled == nothing && cpu_target != nothing) precompiled = "no" end
# or this:
(precompiled == nothing && cpu_target != nothing) && (precompiled = "no")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Julia (like in Bash) && is passed through only if the previous expression returns true, while || is passed through only if the previous expression returns false. So if precompiled = "yes", then precompiled == nothing returns false, and therefore && is not passed through.

Copy link
Member

@NHDaly NHDaly May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) So I think everything you just said is correct, and yet, behold:

julia> false && true || println("HI")
HI

I think the nuance is here:

while || is passed through only if the previous expression returns false.

In this case, the previous expression is actually the truth-value on the left-hand side of the ||. I would imagine it like this:

`precompiled == nothing` --> <evaluate expression ... okay, it's false!> --> `false` --> `&& (cpu_target == nothing)` --> <okay, skip the right-hand side, cause this already evaluates to false --> `false` --> `|| (precompiled = "no")` --> <now we need to evaluate the right-hand side again because the left-hand side was false, so the truthiness of the whole expression depends on the right-hand side> --> (precompiled = "no") --> <returns `nothing`>.

That's why I suggested, if you want to keep it exactly as you currently have it, you need to put parens around the entire right-hand side of &&, so that it can skip the whole thing:

julia> false && (true || println("hello?"))
>> false

But i still think an if-statement might be better, since, clearly, multiple short-circuiting statements can be confusing and hard to read. :)

Copy link
Member

@NHDaly NHDaly May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(In bash as well):

$ false && true || echo "HI"
HI
$ false && (true || echo "helloooooooooo")
$

Copy link
Collaborator Author

@lucatrv lucatrv May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're perfectly right of course :) it's due to the precedence of && over ||, I'll fix that, thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty straightforward:

precompiled == nothing && cpu_target != nothing && (precompiled = "no")

Copy link
Member

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Otherwise, this looks good to me!

@lucatrv
Copy link
Collaborator Author

lucatrv commented May 11, 2018

I'm interested in the use case for disabling/enabling --precompiled and --compilecache. Is there a reason that a user of this package would want that?

We are still in an early stage of development of Static Julia, there are still doubts for instance whether disabling compilecache is required just because of a julia bug or other. Moreover things may change once julia-compile is introduced, which may provide behaviors specifically addressed at static compilation.
So my intent is for now to provide full testing capabilities, and to keep juliac aligned as much as possible with julia (now) and with julia-compile (in future), so that it is possible to verify all possible flags combinations, while keeping safe defaults.

If so, could we maybe also explain that a bit in the docstring?

I agree that in future, once everything is clarified, we could add explanations in the docstring, but for now I thought to leave juliac docstring as aligned as possible with julia, so that we can easily follow its development changes, until everything is clarified.

@lucatrv lucatrv force-pushed the add-other-julia-compilation-flags branch from 280d36c to 5db0587 Compare May 11, 2018 21:05
@codecov-io
Copy link

codecov-io commented May 11, 2018

Codecov Report

Merging #62 into master will decrease coverage by 0.29%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #62     +/-   ##
=========================================
- Coverage   69.83%   69.53%   -0.3%     
=========================================
  Files           5        5             
  Lines         295      302      +7     
=========================================
+ Hits          206      210      +4     
- Misses         89       92      +3
Impacted Files Coverage Δ
src/snooping.jl 78.94% <ø> (ø) ⬆️
src/api.jl 28.57% <ø> (ø) ⬆️
src/static_julia.jl 75.36% <78.57%> (-0.98%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60a4c6a...8c72883. Read the comment docs.

@lucatrv lucatrv force-pushed the add-other-julia-compilation-flags branch from 5db0587 to 8c72883 Compare May 12, 2018 10:18
@lucatrv lucatrv merged commit d2ccb69 into JuliaLang:master May 12, 2018
@lucatrv lucatrv deleted the add-other-julia-compilation-flags branch May 12, 2018 14:30
@NHDaly
Copy link
Member

NHDaly commented May 12, 2018 via email

KristofferC added a commit that referenced this pull request Feb 9, 2020
* Revert "remove github documenter workflow"

This reverts commit dacb45b00005325fc5a04efa62a7646e3bc2c054.

* do not run documenter on travis

* pretty urls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants