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 errors: conrtib/build_{executable,sysimg}.jl #14176

Merged
merged 3 commits into from
Dec 5, 2015

Conversation

KDr2
Copy link
Contributor

@KDr2 KDr2 commented Nov 28, 2015

  • fix few compile error
  • do NOT build both release file and debug file at once, build a release file by default, build a debug version by --debug option
  • add option --portable to build a portable executable file which can be run with LD_LIBRARY_PATH and JULIA_SYSIMG environment var set.

@ViralBShah
Copy link
Member

Cc: @nalimilan

println("running: rm -rf $(tmpdir) $(sys.buildfile).o $(sys.buildfile0).o $(sys.buildfile0).ji")
map(f-> rm(f, recursive=true), [tmpdir, sys.buildfile*".o", sys.buildfile0*".o", sys.buildfile0*".ji"])
println("running: rm -rf $(tmpdir) $(sys.buildfile * ".o") $(sys.buildfile0 * ".o") $(sys.buildfile0 * ".ji")")
run(`rm -rf $(tmpdir) $(sys.buildfile * ".o") $(sys.buildfile0 * ".o") $(sys.buildfile0 * ".ji")`)
Copy link
Contributor

Choose a reason for hiding this comment

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

use the julia rm function, not the coreutils rm which is not going to be present on windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks.

@nalimilan
Copy link
Member

@ViralBShah Did you mean to CC somebody else? I've no particular knowledge about this file.

@tkelman
Copy link
Contributor

tkelman commented Nov 29, 2015

@dhoegh has been pretty actively maintaining a version of this at https://github.com/dhoegh/BuildExecutable.jl - it might be worth actually migrating so there aren't 2 separate copies of this going forward

@dhoegh
Copy link
Contributor

dhoegh commented Nov 29, 2015

Yes I would deferentially appreciate the help at https://github.com/dhoegh/BuildExecutable.jl . I have already fixed some of the issues mentioned here. I support the idea of removing build_executable.jl from base as it is untested in base and the code is self-sustained. It is easier to fix bugs in the script, as BuildExecutable.jl do not need to follow base releases. At BuildExecutable.jl the code is tested on both Travis and Appveyor.

@KDr2
Copy link
Contributor Author

KDr2 commented Nov 30, 2015

@dhoegh great work.
I would like to migrate the code to @dhoegh 's BuildExecutable.jl. But I should do this in my spare time, it may take more or less a week, I'll delete these two files from the julia base repo after that. This PR can be merged and I'll create a new one for the migration/deleting, Or just leave it here util the migration has been done. Is this a working plan? @tkelman @nalimilan @ViralBShah @dhoegh

@dhoegh
Copy link
Contributor

dhoegh commented Nov 30, 2015

My opinion is that build_sysimg.jl belongs in base and only build_executable.jl should be migrated and removed from base. I do only carry the build_sysimg.jl in the repository to fix build_executable.jl on 0.4, see #14018. When 0.4.2 is released I will bump the requirement to 0.4.2 and remove build_sysimg.jl from the repository.

@tkelman
Copy link
Contributor

tkelman commented Nov 30, 2015

That makes sense to me. sysimg is needed if you want to make a change to base source and actually have it take effect, which can be a bit counterintuitive for newcomers.

@KDr2
Copy link
Contributor Author

KDr2 commented Dec 4, 2015

I think this PR is ready to merge, I've migrated the change to @dhoegh's https://github.com/dhoegh/BuildExecutable.jl at dhoegh/BuildExecutable.jl#6 .

@tkelman
Copy link
Contributor

tkelman commented Dec 4, 2015

Note the travis errors, make install needs to be updated for the removal of build_executable.jl. Do a git grep -n build_executable.jl to find if it's mentioned anywhere else.

@KDr2
Copy link
Contributor Author

KDr2 commented Dec 4, 2015

Done, thanks. @tkelman

@tkelman
Copy link
Contributor

tkelman commented Dec 5, 2015

Travis failure was an unrelated repl test issue caused by fb48014. Thanks for the contribution!

tkelman added a commit that referenced this pull request Dec 5, 2015
Fix errors: conrtib/build_{executable,sysimg}.jl
@tkelman tkelman merged commit bac35d0 into JuliaLang:master Dec 5, 2015
@KDr2 KDr2 deleted the executable-builder branch December 5, 2015 12:16
@dhoegh
Copy link
Contributor

dhoegh commented Dec 8, 2015

Could the changes to build_sysimg.jl be backported to 0.4?

@tkelman
Copy link
Contributor

tkelman commented Dec 8, 2015

I guess? The debug flag is a bit feature-y.

@dhoegh
Copy link
Contributor

dhoegh commented Dec 8, 2015

I see. If this got backported I would be able to get rid of the local copy of build_sysimg.jl I carry in https://github.com/dhoegh/BuildExecutable.jl which I would be nice.

@tkelman
Copy link
Contributor

tkelman commented Dec 8, 2015

I thought the osx fix that did get backported was enough for that? What does having the debug flag fix?

@dhoegh
Copy link
Contributor

dhoegh commented Dec 8, 2015

In dhoegh/BuildExecutable.jl#6 @KDr2 states that

If I only have julia or julia-debug in my JULIA_HOME, it fails to build.
I do not know how @KDr2 is building/using julia but apparently in a way, where julia and julia-debug is not contained within JULIA_HOME.

@tkelman
Copy link
Contributor

tkelman commented Dec 8, 2015

In a source build you might only have release but not debug present, or maybe the other way around.

I'm a bit skeptical of some of the changes over there with respect to JULIA_HOME, you should not be setting that environment variable or modifying it.

@KDr2
Copy link
Contributor Author

KDr2 commented Dec 9, 2015

  • About the debug flag: Yeah, I usually build julia from source with make, or make julia-debug occasionally, and sometime I do make clean(all)?, so julia and julia-debug may not exist at the same time.
  • About unsetting/setting JULIA_HOME: I found jl_init_with_image we are using calls julia_init with JL_IMAGE_JULIA_HOME, when JULIA_HOME is set(usually to the bin directory under the julia installation), it will lookup the system image for our built executable under JULIA_HOME, not the directory where the executable placed. so I unset the JULIA_HOME before calling jl_init_with_image and reset it after the call. Maybe there's another better solution?

@tkelman @dhoegh

@tkelman
Copy link
Contributor

tkelman commented Dec 9, 2015

Don't set JULIA_HOME in the first place?

@KDr2
Copy link
Contributor Author

KDr2 commented Dec 9, 2015

Yes, don't set JULIA_HOME in the first place is the right way 😄 , and the c code will do nothing if we have no JULIA_HOME set.

But sometimes the JULIA_HOME is set

  • by accident(I mean not by user's intent, just forget to unset it, or set by a program which exec our executable...)
  • because the julia program which we are packaging is using ENV["JULIA_HOME"] for some purpose.

Or we can simply and clearly write this down into the document.

@tkelman
Copy link
Contributor

tkelman commented Dec 9, 2015

That environment variable is significant to the julia runtime, so it shouldn't be used for arbitrary other purposes. That significance should be documented, or possibly removed if the original reasons for it are no longer applicable.

tkelman pushed a commit that referenced this pull request Jan 9, 2016
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.

5 participants