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

Have change_dir_if_bundle cd to Bundle Root instead of Resources dir #10

Closed
wants to merge 1 commit into from

Conversation

NHDaly
Copy link
Owner

@NHDaly NHDaly commented May 13, 2018

No description provided.

@codecov-io
Copy link

codecov-io commented May 13, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@3ca024b). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #10   +/-   ##
=========================================
  Coverage          ?   72.41%           
=========================================
  Files             ?        3           
  Lines             ?      116           
  Branches          ?        0           
=========================================
  Hits              ?       84           
  Misses            ?       32           
  Partials          ?        0
Impacted Files Coverage Δ
src/ApplicationBuilder.jl 20% <0%> (ø)

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 3ca024b...965b68f. Read the comment docs.

@coveralls
Copy link

coveralls commented May 13, 2018

Pull Request Test Coverage Report for Build 48

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 72.414%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ApplicationBuilder.jl 0 2 0.0%
Totals Coverage Status
Change from base Build 46: 0.0%
Covered Lines: 84
Relevant Lines: 116

💛 - Coveralls

@ranjanan
Copy link
Contributor

LGTM

@NHDaly NHDaly force-pushed the change_dir_path branch from 17fabff to 6d78001 Compare May 13, 2018 19:21
@NHDaly
Copy link
Owner Author

NHDaly commented May 13, 2018

@ranjanan Ah, okay no wait, nvm I no longer think this PR is a good idea. I remember why I had things the way I had them before:

The app will change directories into the Resources directory. Therefore, anything you want to reference directly as a file, you put in Resources.

As for the libraries, the Libraries directory is added to the LD_LIBRARY_PATH during compilation (here), and then added to the compiled binary's rpath (here).

This way, the user code can simply refer to any local files directly, the same as they are relative to the repo directory, in the same way it would when running through regular julia. This is important so that the code works the same whether compiled or not. Here is one example from Paddle Battle:
https://github.com/NHDaly/PaddleBattleJL/blob/v1.0.3/main.jl#L409

Otherwise, those would have to be "Resources/assets" sometimes.

You originally asked this:

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/...

They don't have to refer to the libraries via "../Libraries/...". In any ccall, they can simply write the library name (because of the rpath). If they want to refer to a library by name for some reason outside ccall, it is really a resource, not a library, and should go in -R. What do you think?

Is the problem coming for you for the windows version?

@NHDaly NHDaly force-pushed the change_dir_path branch from 6d78001 to 965b68f Compare May 13, 2018 20:13
@ranjanan
Copy link
Contributor

Yeah, so rpath is specific to Mac AFAIK. In windows, I'm bundling the following way:

root:
- lib
- core
- res

and I change the library paths internally to lib/

Would it be a massive problem to change to Resources/assets/ for some files? Would be consistent on all three platforms.

@ranjanan
Copy link
Contributor

Bump!

@NHDaly
Copy link
Owner Author

NHDaly commented Jun 1, 2018

Hey, sorry, I've been very busy the last few weeks; my girlfriend graduated from grad-school this week, and now we're moving across the US to a new state! I should be more settled soon, hopefully. Thanks for the bump reminder.

Yeah, so rpath is specific to Mac AFAIK

Yeah agreed that rpath is specific to Mac. So you're right that relying on that is probably not a good solution.

and I change the library paths internally to lib/

Just wondering, what is the mechanism you're using for that right now?

Would it be a massive problem to change to Resources/assets/ for some files? Would be consistent on all three platforms.

I agree that consistency on Windows/Linux/Mac is an important concern, we should find a solution that works the same for all of them!

My other concern is trying to maximize consistency between the code for "regular" julia operation and for a compiled application. That is, I want to make sure that the exact same code will work with Juno/Atom/command-line julia and then will also work when it's compiled.

And I also don't want to impose a directory structure on user's repos, which is why I opted for the -R and -L flags.

So however we organize the resultant compiled application, ideally we would cd to a location where the directory structure looks the same as the orginal repo from the code's perspective.

@NHDaly
Copy link
Owner Author

NHDaly commented Jun 1, 2018

To be more clear, my main concern is to limit the number of if IS_COMPILED checks needed throughout the code, and so I think we shouldcd to a directory where the resources are available via the same path that they were in the original repo.

If there is a better solution for the libs, let's look into it!

@NHDaly
Copy link
Owner Author

NHDaly commented Jun 1, 2018

Actually, maybe the answer is simply that there shouldn't be a difference between -L and -R, and they should all go to the same place?

On Mac they would all go into Resources. Then you could cd there and all the libraries could be referenced by simply their basename.
And on Windows, they would all go into something like res/?

@NHDaly
Copy link
Owner Author

NHDaly commented Feb 17, 2022

Closing this stale PR

@NHDaly NHDaly closed this Feb 17, 2022
@NHDaly NHDaly deleted the change_dir_path branch February 17, 2022 04:41
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.

4 participants