Attempting to move build_app.jl code into a function inside ApplicationBuilder. #9

NHDaly · 2018-05-11T17:35:33Z

No description provided.

NHDaly · 2018-05-11T17:47:19Z

So, this change breaks things.

Basically, the problem comes from mixing two services in the same Module: providing runtime code for the built applications to call (change_dir_if_bundle()) and providing buildtime code to create those applications (build_app_bundle()).

More specifically, the problem is that, after this change, the ApplicationBuilder module calls using PackageBuilder, in order to call static_julia. However, this means that during compilation, when the user application code calls using ApplicationBuilder, it also attempts to import PackageCompiler, whose build.jl check for GCC fails:

WARNING: redefining constant JULIA_HOME
WARNING: Your Julia system image is not compiled natively for this CPU architecture.
        Please run `PackageCompiler.force_native_image!()` for optimal Julia performance
ERROR: LoadError: LoadError: LoadError: LoadError: GCC wasn't found. Please make sure that gcc is on the path and run Pkg.build("PackageCompiler")

NHDaly · 2018-05-11T18:03:19Z

These are some solutions I can think of:

Add ENV["COMPILING_APPLE_BUNDLE"] == "false" checks around build_app_bundle() (including the using Glob, PackageCompiler statement), so that the build-time code isn't defined if this is being included from within a program being compiled.
- downside: The buildtime-only code will still be included from the app when run through the julia interpreter, unnecessarily increasing load-times.
- downside: Prevents building an Application which, itself, builds applications. (probably not a concern for now, but i could imagine a gui-based ApplicationBuilder being actually fairly useful.)
Separate the runtime and buildtime code into two different Packages, perhaps ApplicationBuilder and ApplicationUtils?
- downside: increased maintenance, complexity burden. Users will have to be sure to install both.
Separate the runtime and buildtime code into different Modules, defined within the same Package. Apparently that's a thing you can do. The parent package could perhaps (boldly) be called Application.jl, and it could simply add two modules, AppBuilder and AppUtils, to the LOAD_PATH, ready to be imported.
- downside: slight increase in user complexity. Will always have to write using Application; using AppUtils, which is sort of weird...
Same as above, but only with one extra Module. Keep the runtime code in the top-level module or a submodule. This way the user application code will still be a normal using statement. Then, just separate out the build_app_bundle() code into its own Module. This way, only build-time code will have the weird using statement.
- downside: Naming the packages and modules will be tough. Perhaps using Application.Utils for the runtime code and using Application; using AppBuilder for the buildtime code?
Somehow change PackageCompiler to not perform this check for gcc?

My gut says either the separate Modules or separate Packages solutions are probably best?
@lucatrv @ranjanan: I'd really appreciate your input! This is a blocker to the current structure of PR #7.

lucatrv · 2018-05-11T20:05:56Z

Sorry I cannot provide a good contribution on this as I do not have enough experience with ApplicationBuilder. As for me I would also consider option 5 above, but this would have to be reviewed by @SimonDanisch who wrote that part of PackageCompiler.

ranjanan · 2018-05-13T13:35:27Z

Sorry, are you running into the problem mentioned here ? I think I ran into the GCC error because of that. I thought you had fixed those in #4 ?

NHDaly · 2018-05-13T15:54:50Z

Ah, no, sorry, this is unrelated to #4. In the current state of this PR (273ea73), the problem is as follows:

First, the user uses ApplicationBuilder.build_app to create their compiled application (julia build_app.jl examples/hello.jl)
This calls PackagerCompiler.build_executable, which eventually launches a new julia process to do the actual compilation.
The new julia process executes the user's application code (ex, examples/hello.jl). This code has as its first line using ApplicationBuilder, which itself has using PackageCompiler, and for some reason this crashes trying to load gcc here.

I'm not sure why that fails, but it shouldn't really matter -- there's no reason for the user's application code to be running using PackageCompiler in the first place. It's only due to this organizational decision that they happen to live in the same Module.

Does that make more sense?

ranjanan · 2018-05-13T16:12:07Z

Right, I understand now. Does this happen to be same issue? Sorry, I made a mistake copying the right link in the previous comment.

NHDaly · 2018-05-13T16:39:26Z

Ah, sorry, yes it's exactly what you described here:

I just realised. The problem with doing import ApplicationBundler: change_dir_if_bundle() and then using it in code is that PackageCompiler would try to statically compile ApplicationBundler too, which in turn depends on PackageCompiler. Not sure how well that work.....

You explained it much more succinctly than me. :)

NHDaly · 2018-05-13T16:42:33Z

So, I've gone ahead and tried solution number 4 above, and that did fix things. (I think the test failure was just a flake; i'm retrying it now.)

It's here in this commit i just pushed: 7f246bd

Does it seem overly complicated to you? It prevents exactly the cycle you described in your link. The tests all pass locally now. :)

coveralls · 2018-05-13T16:54:15Z

Pull Request Test Coverage Report for Build 45

83 of 103 (80.58%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+52.4%) to 72.414%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ApplicationBuilder.jl	1	9	11.11%
src/BuildApp.jl	82	94	87.23%

Totals
Change from base Build 27:	52.4%
Covered Lines:	84
Relevant Lines:	116

💛 - Coveralls

ranjanan · 2018-05-13T17:06:27Z

src/ApplicationBuilder.jl

+                # Can find the code's path from what the full_binary_name ends in.
+                m = match(r".app/Contents/MacOS/[^/]+$", full_binary_name)
+                if m != nothing
+                    resources_dir = joinpath(dirname(dirname(full_binary_name)), "Resources")


So I was wondering if you wanted to set the default path to the Resources directory or straight to the bundle root, so now the user would have to specify paths to libraries by "Libraries/..." instead of "../Libraries/...

Yeah i think that makes lots of sense. Want to discuss that in a separate thread?

Sure, wherever you prefer. :-) I do this by default in #7 because it makes things convenient.

ranjanan · 2018-05-13T17:11:49Z

src/BuildApp.jl

+
+include("sign_mac_app.jl")
+
+function build_app_bundle(parsed_args)


I was wondering if build_app_bundle could be of a similar API as described in #7. Since the user is primarily going to be interacting with this function, perhaps it would be nice if he can specify an array of paths to libraries and array of paths to resources. What do you think? :-)

yes, absolutely. This commit was just the first step (the PR was work-in-progress). Sorry that wasn't clear!

I was just specifically hoping to get your feedback on splitting the Modules.

That said, i've just pushed up a commit that makes this cleaner!

Ah, yeah, let me make the resources and libraries args a bit nicer, like the way you have them.

Otherwise, I think the API looks like you have it in #7 now? Yes, the goal of this PR was exactly that, to make the API match what you had in #7 so that the windows and mac versions could be the same, since I agree the API as you have it is the nicest! :)

ranjanan · 2018-05-13T17:19:47Z

I think Solution 4 is the best too. Forgive me, but could I clarify one thing? With this solution, are you saying that the build script would look like this:

using ApplicationBundler

libs = [...]
res = [...]

build_app_bundle("path/to/script_to_be_compiled.jl", libraries = libs, resources = res)

and the script_to_be_compiled.jl would be:

function Base.@ccallable julia_main(ARGS)
 ApplicationBundler.Utils.chdir_if_bundle()
...
end

?

Do I understand correctly?

NHDaly · 2018-05-13T17:23:44Z

I think Solution 4 is the best too.

Okay great. Thanks! That's how it is now.

With this solution, are you saying that the build script would look like this?

Yes, almost exactly. The only modification to your example is that for the build-script, using ApplicationBundler would instead have to be using ApplicationBundler; using BuildApp, or whatever we call the second Module.

That's how it looks now:

ApplicationBuilder.jl/build_app.jl

Line 1 in 14fae3e

using ArgParse, ApplicationBuilder; using BuildApp

NHDaly · 2018-05-13T17:24:32Z

And the application code is exactly as you have it:

ApplicationBuilder.jl/examples/hello.jl

Lines 4 to 5 in 14fae3e

    
           Base.@ccallable function julia_main(ARGS::Vector{String})::Cint 
        
               ApplicationBuilder.App.change_dir_if_bundle()

ranjanan · 2018-05-13T17:32:17Z

And the application code is exactly as you have it:

Oh I see. So you're saying that if we put change_dir_if_bundle in a second module, PackageCompiler will compile it fine because PackageCompiler itself isn't in that module, even though it's in the package. Nice 😄

NHDaly · 2018-05-13T17:37:49Z

Oh I see. So you're saying that if we put change_dir_if_bundle in a second module, PackageCompiler will compile it fine because PackageCompiler itself isn't in that module. Nice 😄

Haha yep that's exactly it! Does that make sense? It feels sort of hacky... but idk it's only for the build-script so maybe that's not too terrible? There doesn't seem to be any other way to group together related modules in a single package. I think this isn't bad -- at least for now! :)

Although, just for clarity, change_dir_if_bundle doesn't need to be in a submodule of ApplicationBuilder; it would work just as well if that was in ApplicationBuilder. The important part is that PackageCompiler is in a separate module so that it doesn't compile itself, as you say. :)

I just thought it was clearer to put change_dir_if_bundle in a submodule. Since we're moving the build code out of the top-level, moving the application utils out of the top-level too seemed clearest as well. That way, the top-level package module simply defines the other two modules.

ranjanan · 2018-05-13T17:44:03Z

I think this isn't bad -- at least for now! :)

For sure, I agree. 👍

I think this PR looks good now, I shall refactor #7 to do the same.

NHDaly · 2018-05-13T17:45:19Z

!! thanks! :) You should be able to refactor off of this branch.

Actually, if you think it's okay now, do you want me to go ahead and merge then?

NHDaly · 2018-05-13T17:49:24Z

~~For me, the remaining open questions are just surrounding the names for the modules, but maybe i can just move these to a new Issue...~~

EDIT:
I've moved the names question to #11.

ranjanan · 2018-05-13T17:53:49Z

Go ahead and merge, once tests pass. No, I think the package can stay as ApplicationBuilder. I think the submodule with build_app_bundle should be Builder. And change_dir_if_bundle can stay in BuildUtils

NHDaly · 2018-05-13T17:55:49Z

Okay thanks i will. I'm copying your comment over to #11 as well. Sorry!

Add a new submodule (`App`) for "runtime" utilities needed by applications built through this package. This is in prepration for moving the body of build_app.jl inside the main ApplicationBuilder module.

(This is a simple copy/paste with no modifications.)

The runtime utilities for user application code will be evaluated in the top-level module, and build-time code in a separate module. This prevents build-time code from being evaluated during compilation of the user application code, accidentally.

Make all the args explicit, with well defined defaults, and change build_app.jl to simply parse user args and then pipe them to `BuildApp.build_app_bundle(juliaprog_main; parsed_args...)`.

Renames `test/examples.jl` to `test/build_app-cli.jl` since it's testing the actual command line interface to the code. Changes the test file to `include` the code instead of spawn a child process to calculate more accurate coverage info.

test/examples.jl -> test/BuildApp.jl and test/build_app-cli.jl

…pp.jl

codecov-io · 2018-05-13T19:56:37Z

Codecov Report

Merging #9 into master will increase coverage by 52.41%.
The diff coverage is 80.58%.

@@             Coverage Diff             @@
##           master       #9       +/-   ##
===========================================
+ Coverage      20%   72.41%   +52.41%     
===========================================
  Files           1        3        +2     
  Lines          10      116      +106     
===========================================
+ Hits            2       84       +82     
- Misses          8       32       +24

Impacted Files	Coverage Δ
src/sign_mac_app.jl	`0% <ø> (ø)`
src/ApplicationBuilder.jl	`20% <11.11%> (ø)`	⬆️
src/BuildApp.jl	`87.23% <87.23%> (ø)`

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 4f1304d...2e90268. Read the comment docs.

NHDaly · 2018-05-13T20:05:38Z

Rebased and merged! :) (look at that Coverage bump! haha wooot)

NHDaly · 2018-05-13T20:09:33Z

We should eventually also probably split this into a bunch of smaller functions, since that will make probably make swapping out the different OS functionality easier.. but that is a task for the future! :p

EDIT:
tracked in #12

ranjanan reviewed May 13, 2018

View reviewed changes

NHDaly force-pushed the package_restructure branch from e99d3f0 to 4f6df98 Compare May 13, 2018 17:36

NHDaly mentioned this pull request May 13, 2018

Consider renaming modules after #9 #11

Closed

3 tasks

NHDaly added 7 commits May 13, 2018 13:09

Move change_dir_if_bundle() to ApplicationBuilder.App.

b579756

Add a new submodule (`App`) for "runtime" utilities needed by applications built through this package. This is in prepration for moving the body of build_app.jl inside the main ApplicationBuilder module.

Moved body of build_app.jl to ApplicationBuilder.build_app_bundle()

c82dad3

(This is a simple copy/paste with no modifications.)

Make build_app_bundle a proper function interface.

ee1eeaa

Make all the args explicit, with well defined defaults, and change build_app.jl to simply parse user args and then pipe them to `BuildApp.build_app_bundle(juliaprog_main; parsed_args...)`.

Normalize test/examples: call code directly.

3b65639

Renames `test/examples.jl` to `test/build_app-cli.jl` since it's testing the actual command line interface to the code. Changes the test file to `include` the code instead of spawn a child process to calculate more accurate coverage info.

Add test for function API! (forked from test/examples.jl)

230d23a

Fork test/examples.jl into API and cli tests.

1965df6

test/examples.jl -> test/BuildApp.jl and test/build_app-cli.jl

NHDaly force-pushed the package_restructure branch from 4f6df98 to 93cc079 Compare May 13, 2018 19:11

simplified test/build_app-cli.jl to not repeat tests from test/BuildA…

2e90268

…pp.jl

NHDaly force-pushed the package_restructure branch from 93cc079 to 2e90268 Compare May 13, 2018 19:34

NHDaly merged commit 2e90268 into master May 13, 2018

NHDaly deleted the package_restructure branch May 13, 2018 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempting to move build_app.jl code into a function inside ApplicationBuilder. #9

Attempting to move build_app.jl code into a function inside ApplicationBuilder. #9

NHDaly commented May 11, 2018

NHDaly commented May 11, 2018

NHDaly commented May 11, 2018

lucatrv commented May 11, 2018

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

NHDaly commented May 13, 2018

coveralls commented May 13, 2018 •

edited

Loading

ranjanan May 13, 2018

NHDaly May 13, 2018

NHDaly May 13, 2018 •

edited

Loading

ranjanan May 13, 2018

ranjanan May 13, 2018

NHDaly May 13, 2018

NHDaly May 13, 2018

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

NHDaly commented May 13, 2018

ranjanan commented May 13, 2018 •

edited

Loading

NHDaly commented May 13, 2018 •

edited

Loading

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

NHDaly commented May 13, 2018 •

edited

Loading

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

codecov-io commented May 13, 2018 •

edited

Loading

NHDaly commented May 13, 2018

NHDaly commented May 13, 2018 •

edited

Loading


		include("sign_mac_app.jl")

		function build_app_bundle(parsed_args)

Attempting to move build_app.jl code into a function inside ApplicationBuilder. #9

Attempting to move build_app.jl code into a function inside ApplicationBuilder. #9

Conversation

NHDaly commented May 11, 2018

NHDaly commented May 11, 2018

NHDaly commented May 11, 2018

lucatrv commented May 11, 2018

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

NHDaly commented May 13, 2018

coveralls commented May 13, 2018 • edited Loading

Pull Request Test Coverage Report for Build 45

💛 - Coveralls

ranjanan May 13, 2018

Choose a reason for hiding this comment

NHDaly May 13, 2018

Choose a reason for hiding this comment

NHDaly May 13, 2018 • edited Loading

Choose a reason for hiding this comment

ranjanan May 13, 2018

Choose a reason for hiding this comment

ranjanan May 13, 2018

Choose a reason for hiding this comment

NHDaly May 13, 2018

Choose a reason for hiding this comment

NHDaly May 13, 2018

Choose a reason for hiding this comment

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

NHDaly commented May 13, 2018

ranjanan commented May 13, 2018 • edited Loading

NHDaly commented May 13, 2018 • edited Loading

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

NHDaly commented May 13, 2018 • edited Loading

ranjanan commented May 13, 2018

NHDaly commented May 13, 2018

codecov-io commented May 13, 2018 • edited Loading

Codecov Report

NHDaly commented May 13, 2018

NHDaly commented May 13, 2018 • edited Loading

coveralls commented May 13, 2018 •

edited

Loading

NHDaly May 13, 2018 •

edited

Loading

ranjanan commented May 13, 2018 •

edited

Loading

NHDaly commented May 13, 2018 •

edited

Loading

NHDaly commented May 13, 2018 •

edited

Loading

codecov-io commented May 13, 2018 •

edited

Loading

NHDaly commented May 13, 2018 •

edited

Loading