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

Use INSTALL_TARGET_PROCESSES in Makefile #8

Closed
wants to merge 4 commits into from
Closed

Use INSTALL_TARGET_PROCESSES in Makefile #8

wants to merge 4 commits into from

Conversation

leptos-null
Copy link
Member

As per #2 (mark 5): "Make templates use ... INSTALL_TARGET_PROCESSES to kill processes instead of adding the rule at the bottom."

@uroboro
Copy link
Member

uroboro commented Jul 22, 2018

Thanks for working on this. Can you change it a bit so as to place @@KILL_PROCS@@ after the last variable and treating it as a list?

Taking the tweak example:

include @@THEOS@@/makefiles/common.mk

TWEAK_NAME = @@PROJECTNAME@@
@@PROJECTNAME@@_FILES = Tweak.xm

include $(THEOS_MAKE_PATH)/tweak.mk

It would become:

include @@THEOS@@/makefiles/common.mk

TWEAK_NAME = @@PROJECTNAME@@
@@PROJECTNAME@@_FILES = Tweak.xm
INSTALL_TARGET_PROCESSES = @@KILL_PROCS@@

include $(THEOS_MAKE_PATH)/tweak.mk

@leptos-null
Copy link
Member Author

place @@KILL_PROCS@@ after the last variable

Yes, I'll do that.

An afterthought: Would @@PROJECTNAME@@_INSTALL_TARGET_PROCESSES work/be preferable? I'm not as familiar with make: ref

treating it as a list

I do not think I understand, sorry. Does perl have an array indicator, that which I am not using? I tested this PR by listing multiple processes in NIC (mediaserverd backboardd locationd), and it worked as expected: ref

@kirb
Copy link
Member

kirb commented Jul 23, 2018

Note due to the ifeq I use to check INSTALL_TARGET_PROCESSES is non-empty, it has to be set before including common.mk. I could change that to be tested using a shell if [[ (evaluate when run rather than at include time) as well as maybe adding a per-instance var as a feature.

@leptos-null
Copy link
Member Author

That is what I had originally thought. Collaborator:@uroboro requested I move it to the new location.

I'm not familiar with theos edict. How would you like me to proceed?

If commit 82ece5a is reverted, it should be noted that I fixed the application template that I had missed: ref

@uroboro
Copy link
Member

uroboro commented Jul 23, 2018

At the time I was not aware that the check happened in common.mk. I apologize for the confusion.

@leptos-null
Copy link
Member Author

leptos-null commented Jul 23, 2018

No reason for apologies, I feel this was a productive discussion; additionally, I apologize, as I did not intend to "point any fingers", as to say. If I may recommend, as @kirb mentioned, I think the variable should be a "per-instance var", as he described- similar to the CODESIGN_FLAGS variable.

kirb pushed a commit that referenced this pull request Jul 23, 2018
@kirb
Copy link
Member

kirb commented Jul 23, 2018

Merged via rebase in 48854f2, moved the @@KILL_PROCS@@ back to the top. Thanks!

@kirb kirb closed this Jul 23, 2018
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.

3 participants