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

[DON'T MERGE YET] Update addon_config.mk, remove extraneous info #3

Merged
merged 2 commits into from
Jan 12, 2016

Conversation

mattfelsen
Copy link

Finally got the process of dylibs in addons worked out! This PR updates addon_config and removes a bunch of commented out info. Having only necessary info in the file makes it much easier to find the necessary parts, I think.

Following the merging of this projectGenerator PR this should be good to go.

ADDON_DATA wasn't actually implemented before so nothing would be copied, and it would've gone into the bin/data/ instead of bin/ where it needed to be, so that didn't make sense anyway.

ADDON_LDFLAGS would set up the dylib to be linked against, but it would still need to be manually copied into the project's bin/data/ dir (or fail at runtime)

ADDON_DLLS_TO_COPY is the answer: it both sets up the project for linking this lib, and copies it to bin/. Kind of a weird name though in the OS X context, so I'm pushing to have it renamed to something like ADDON_DYNAMIC_LIBS.

On Windows the libs are static, so none of this is relevant.

@robotconscience
Copy link
Owner

Sweet! Is this still "don't merge", or do you feel good about it?

@mattfelsen
Copy link
Author

Still "don't merge" 😄 I'm waiting on the PR to get merged into the projectGenerator, which implements ADDON_DLLS_TO_COPY in Xcode projects (currently it's ignored for Xcode projects and only works with VS).

@robotconscience
Copy link
Owner

Should prob mention this on the PG pr, but shouldn't it copy the dll into the app package? That's always been my pref.

@mattfelsen
Copy link
Author

Yea, that discussion already started on the PG PR. I was actually surprised putting .dylibs next to the .app bundle worked, rather than inside the .app/Contents/MacOS folder. Since it's the same behavior then for .dlls next to .exes and .dylibs next to .apps, I'm content to leave it alone rather than trying to embed, but go for it if you want to keep the bin folder clean 😝

@robotconscience
Copy link
Owner

Hey dude, what's the status of this PR these days?

@mattfelsen
Copy link
Author

Good question! The PR I submitted hasn't been merged, because as it turns out the dylib problem is more complicated than I thought. Let me test how this is working with and without ADDON_LDFLAGS (I think the pg will do its thing without this flag being explicitly set), but either way ADDON_DLLS_TO_COPY will get dropped and I'll push another commit that should be merge-ready 👍

...still on my list somewhere is to unify all the forks of this addon back to a single place 😑

@robotconscience
Copy link
Owner

That would be dreamy. Happy to join you on this quest...

On Mon, Jan 11, 2016 at 9:45 AM Matt Felsen [email protected]
wrote:

Good question! The PR I submitted hasn't been merged, because as it turns
out the dylib problem is more complicated than I thought. Let me test how
this is working with and without ADDON_LDFLAGS (I think the pg will do
its thing without this flag being explicitly set), but either way
ADDON_DLLS_TO_COPY will get dropped and I'll push another commit that
should be merge-ready [image: 👍]

...still on my list somewhere is to unify all the forks of this addon back
to a single place [image: 😑]


Reply to this email directly or view it on GitHub
#3 (comment)
.

Brett Renfer
\ robotconscience.com
// tsps.cc

@mattfelsen
Copy link
Author

Alrighty, verified that the pg does the right thing for osx/xcode without needing any explicit flags. So this PR has been reduced from "make addons with dylibs work, finally" to a mere "remove bloat from addon_config.mk," but I think that's a worthwhile change anyway, for the sake of readability 😄 Good for merging, methinks

robotconscience added a commit that referenced this pull request Jan 12, 2016
Update addon_config.mk, remove extraneous info
@robotconscience robotconscience merged commit 2495246 into robotconscience:0.9.0 Jan 12, 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.

2 participants