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

Fix #5218, Compile age in banner #5281

Merged
merged 1 commit into from
Jan 2, 2014
Merged

Conversation

ivarne
Copy link
Sponsor Member

@ivarne ivarne commented Jan 2, 2014

After static compillation of base Julia was implemented, the startup
banner showed the age of the newest commit in (origin/)master at the
time of compillation, instead of at startup time. This commit moves the
age calculation into the Base.banner() function, and moves the function
from base/client.jl to base/version.jl in order to have access to the
right variables.

CC: @staticfloat

@staticfloat
Copy link
Sponsor Member

I like the idea, but I think we can do better than search/replacing a magic word in the banner. I have a slightly simpler PR in #5282 that just puts almost everything inside of a function, so we don't have to worry about splitting the work up into pre-compile and post-compile.

@ivarne
Copy link
Sponsor Member Author

ivarne commented Jan 2, 2014

Yes, it is probably not that much overhead in doing everything on start up. It will anyway only get executed when using the REPL.

@staticfloat
Copy link
Sponsor Member

It's still getting precompiled, as well. It's just not getting precached.

After static compillation of base Julia was implemented, the startup
banner showed the age of the newest commit in (origin/)master at the
time of compillation, instead of at startup time. This commit moves the
age calculation into the Base.banner() function, and moves the function
from base/client.jl to base/version.jl in order to have access to the
right variables.

This is updated after @staticfloat suggestion to copy all the logic into
the banner() function
@staticfloat
Copy link
Sponsor Member

I'd like to change that print() to a warn(), so that it's coming out of STDERR.

Other than that, I'd say this is good to merge.

@ivarne
Copy link
Sponsor Member Author

ivarne commented Jan 2, 2014

I think print -> warn is a different issue, unrelated to this.

After static compilation this error is not even printed when it triggers. When the git version PR gets merged, we already give a warning during compilation if git is unavailable, and I hope someone will notice if we put an invalid VERSION in the repository.

Also one would need change the include order in sysimg.jl because warn is not defined when version.jl is executed. If you think this is an important warning we must be clear on when it should be issued. Would println(STDERR, "while creating Base.VERSION, ignoring error $e") be good enough? (if it was not ignored)

@staticfloat
Copy link
Sponsor Member

Let's leave it for the git version PR then. It was more a matter of principle, and I figured that since we've change so much of this file we might as well change that as well. In general, I'd like our error and warning reporting mechanisms to be as consistent as possible, but it's not a blocking issue at all.

@staticfloat
Copy link
Sponsor Member

Can you merge, or should I do it?

@ivarne
Copy link
Sponsor Member Author

ivarne commented Jan 2, 2014

I agree with the consistency issue. Reporting warnings during compilation is different from runtime warnings and I don't know if we have/ should have a standard way of reporting those. Are there many of them?

I can't merge anything because I don't have commit access, so you have to pull the trigger for me.

staticfloat added a commit that referenced this pull request Jan 2, 2014
@staticfloat staticfloat merged commit e3ea35b into JuliaLang:master Jan 2, 2014
@ivarne ivarne deleted the splash-fix branch January 2, 2014 22:45
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.

2 participants