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

Add Makefile for installing framework #214

Merged
merged 1 commit into from
Jan 3, 2018
Merged

Add Makefile for installing framework #214

merged 1 commit into from
Jan 3, 2018

Conversation

keith
Copy link
Contributor

@keith keith commented Dec 28, 2017

This allows you to run make install with optional environment variables
in order to build and install Chisel.framework.

Chisel/Makefile Outdated
DSTROOT="$(TEMPORARY_FOLDER)" \
INSTALL_PATH=/ \
SKIP_INSTALL=NO
install -d "$(PREFIX)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this do anything beyond mkdir -p?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed with above changes

Chisel/Makefile Outdated
-sdk iphonesimulator \
install \
DSTROOT="$(TEMPORARY_FOLDER)" \
INSTALL_PATH=/ \
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know much about Xcode's install action. Why install into a temp path and then copy from there into the true destination?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, nice I didn't re-think this after I made some other changes, updated

@kastiglione
Copy link
Contributor

I should hopefully be able to accept this soon. In the mean time, I have a couple questions:

  • Should we build a fat binary with x86 too?
  • Should we code sign after copying into the destination?

I'm indifferent about the fat binary, I'm fine with being lazy and waiting for a request/need.

I don't know the ins and outs of code signing, but I hit some issues that frustrated me during development of this library. If I loaded the library from where it was originally built and code signed (within the build directory) and then later moved or copied the framework to some install location, then I would no longer be able to load the library from the installed location. The error messages were non-existent or cryptic at best, nothing that I could solve from google. Eventually I figured out that if I re-codesigned the installed library, then I wouldn't have those same loading problems. I guess what I'm asking is, is signing supposed to be post-install or can it be pre-install? Presumably pre-install, but the issues tell me that I don't really know how path comes into play with code signing verification.

@keith
Copy link
Contributor Author

keith commented Dec 29, 2017

Should we build a fat binary with x86 too?

This actually is building both right now, since it's a release build, ONLY_ACTIVE_ARCH is NO.

Should we code sign after copying into the destination?

Good question. The adhoc signing that's happening when I run the install now is currently working for me, but I've also only tested on the simulator. If I remove that signature I do get this for dlerror():

(char *) $3 = 0x00007fa68b400249 "dlopen(/usr/local/opt/chisel/lib/Chisel.framework/Chisel, 2): no suitable image found.  Did find:\n\t/usr/local/opt/chisel/lib/Chisel.framework/Chisel: required code signature missing for '/usr/local/opt/chisel/lib/Chisel.framework/Chisel'\n"

@kastiglione
Copy link
Contributor

I should be able to merge this next week.

@kastiglione kastiglione self-assigned this Jan 2, 2018
Chisel/Makefile Outdated
-sdk iphonesimulator \
install \
DSTROOT="$(PREFIX)" \
INSTALL_PATH=/ \
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the docs, the above two should be flipped. DSTROOT=/ and INSTALL_PATH=$(PREFIX).

Chisel/Makefile Outdated
install \
DSTROOT="$(PREFIX)" \
INSTALL_PATH=/ \
SKIP_INSTALL=NO
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be set to NO in the .xcodeproj? I'm fine with it here either way, but I'm wondering why not have it default to this anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll set it in the project since there's no reason to archive it anyways

This allows you to run `make install` with optional environment variables
in order to build and install Chisel.framework.
@keith
Copy link
Contributor Author

keith commented Jan 2, 2018

Addressed your feedback!

@kastiglione kastiglione merged commit c477a65 into facebook:master Jan 3, 2018
@keith keith deleted the ks/install-framework branch January 3, 2018 00:52
@keith
Copy link
Contributor Author

keith commented Jan 3, 2018

Thanks!

@kastiglione
Copy link
Contributor

You're welcome, thanks for the pull request. I'll create a pull request to add an implementation for the findinstances command some time later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants