-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Make Makefile more portable #2
Make Makefile more portable #2
Conversation
Examples/BasicExample/Makefile
Outdated
@@ -1,21 +1,33 @@ | |||
# Declares the root of the repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be clear which repository you're talking about here, because this file is in the example, (and could have been copied out).
# Declares the root of the repository. | |
# Declares the root of the PlaydateKit repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might make developers more confused as to the difference between PDKIT_ROOT
and REPO_ROOT
. Maybe this can be changed to "this source code repository"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, sorry, I think I got confused. If this is meant to be the root of the current project, what is it used for? I think you need to update the include
line below to refer to PDKIT_ROOT
also!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I'll update the include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know how useful my "approval" is here, but I'll give it! 😉
$(SWIFT_EXEC) $(SWIFT_FLAGS) $(SWIFT_FLAGS_SIMULATOR) -c $^ -emit-module -o $@ | ||
|
||
# MARK: - Build BasicExample Swift Object | ||
build/basicexample_device.o: Sources/BasicExample/*.swift | build/Modules/playdatekit_device.o | ||
build/basicexample_device.o: Sources/$(PRODUCT_NAME)/*.swift | build/Modules/playdatekit_device.o | ||
$(SWIFT_EXEC) $(SWIFT_FLAGS) $(SWIFT_FLAGS_DEVICE) -c $^ -o $@ | ||
$(OBJDIR)/pdex.elf: build/basicexample_device.o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How important are all these .o
filenames? Should we introduce a variable (like $(PRODUCT_NAME_LOWERCASE)
) for them too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure on how the .o files play into it, but I had been thinking about this as well. However, I'd like to see if there's a way we can have Make automatically make a lowercase version instead of us developers needing to do this. I'm not an expert on Make, so that might be challenging. Perhaps this can be tackled in a different PR once we have the proper knowledge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the object file doesn't matter as long as you pass it to the linker for the appropriate build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting, it doesn't so much matter what they are, but the cache folder is shared (because of the "parent" make file), so if you do copy the BasicExample folder, and don't change these names, and you've built and run the BasicExample, you'll get some funky warnings. I had to clean my build folder and change these names before my the warnings went away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, I think this is a good change, thanks! At the moment though there is one issue, the build-and-run.sh script reads PRODUCT using sed, but now you have another variable nested inside PRODUCT which isn't replaced. This is causing the Playdate Simulator to not launch the game when run from Xcode. We'll need a more sophisticated way to read the product in that script.
the broken line:
PRODUCT=$(sed -n '/^PRODUCT :=/s///p' Makefile)
My bad, I don't use Xcode for Swift packages regularly (I use Nova, personally). I'll see if I can get a fix in |
To help make the Makefile more portable for different projects, the PlaydateKit path is now extracted to PDKIT_ROOT, as it may not always be the same as REPO_ROOT. PRODUCT_NAME was also introduced to make changing the source paths for game source code easier, and PRODUCT uses PRODUCT_NAME to its advantage. Documentation on these variables are provided as comments here. In the future, command line tools can help set these variables automatically (such as with CMake) to automatically generate this Makefile.
92336fd
to
01ae78a
Compare
To help make the Makefile more portable for different projects, the PlaydateKit path is now extracted to
PDKIT_ROOT
, as it may not always be the same asREPO_ROOT
.PRODUCT_NAME
was also introduced to make changing the source paths for game source code easier, andPRODUCT
usesPRODUCT_NAME
to its advantage.Documentation on these variables is provided as comments here. In the future, command line tools can help set these variables automatically (such as with CMake) to automatically generate this Makefile.