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

Create cmake target ppfw #1716

Merged
merged 3 commits into from
Jan 4, 2024
Merged

Conversation

u-foka
Copy link
Contributor

@u-foka u-foka commented Jan 4, 2024

It will build an OCI ppfw package to be flashed or shared on test-drive

Additionally some minor cmake cleanup
And updated gitignore to allow multiple build folders

The dev builds are named after the current time to avoid numbering the files by the OS at the wrong place. 😇 Ended up using time instead of VERSION_MD5 as its just calculated from the current commit hash so might not be unique if some dev experiments with something and shares multiple builds without a commit in between. Also its minute resolution only, I don't think there would be multiple builds shared within the same minute 🙈

For now it's a duplicate. CI still uses its own method, but wanted to get it out to fellow devs as fast as possible :)

It will build an OCI ppfw package to be flashed or shared on test-drive

Additionally some minor cmake cleanup
And updated gitignore to allow multiple build folders
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@NotherNgineer
Copy link
Contributor

NotherNgineer commented Jan 4, 2024

Probably a dumb question, but should a tar file result when I build using these new Cmake files?

I don't see any tar file generation code in the Makefile's after running cmake (nor is a ppfw.tar file generated).

Steps I followed: Pulled down this "release_with_cmake" code branch from github; used chown -R to make all writable; created build subdirectory and cd'd to it; ran "cmake .."; ran "make".

@u-foka
Copy link
Contributor Author

u-foka commented Jan 4, 2024

Probably a dumb question, but should a tar file result when I build using these new Cmake files?

I don't see any tar file generation code in the Makefile's after running cmake (nor is a ppfw.tar file generated).

Steps I followed: Pulled down this "release_with_cmake" code branch from github; used chown -R to make all writable; created build subdirectory and cd'd to it; ran "cmake .."; ran "make".

Ah sorry didnt mention above, I didnt make it default target, so you have to "make ppfw" or "ninja ppfw" to build the package that will be in the firmware folder.

Should we make it default?

Should we rename the target to OCI? 🙈🤷‍♂️

@NotherNgineer
Copy link
Contributor

That works -- thanks!

I would recommend making the tar file by default. It adds less than a second to the build.

I don't know what OCI stands for, so I guess I can't answer the rename question.

@u-foka
Copy link
Contributor Author

u-foka commented Jan 4, 2024

Made the ppfw target default

Also created an alternate oci target so both make oci and make ppfw can be used to explicitly create the package

@NotherNgineer
Copy link
Contributor

Works great! But I would comment that the file names are getting a bit long for the Flash utility (see photo):

SCR_0084

@u-foka
Copy link
Contributor Author

u-foka commented Jan 4, 2024

Works great! But I would comment that the file names are getting a bit long for the Flash utility (see photo):

Agreed.. :/ Should we just call it pp-oci-240104-2238.ppfw.tar? :)

@htotoo
Copy link
Member

htotoo commented Jan 4, 2024

i wouldn't stick to -oci-. Users will learn that ppfw.tar is oci :D or as you mentioned ppfw could be renamed to oci.tar.
Also date is ok, but the 2238 is the time? do we need it?

@u-foka
Copy link
Contributor Author

u-foka commented Jan 4, 2024

OK I gonna just merge this for now, anyone can change the name easily if theres a consensus :)

@u-foka u-foka merged commit 9e61f80 into portapack-mayhem:next Jan 4, 2024
3 checks passed
@u-foka u-foka deleted the release_with_cmake branch January 4, 2024 22:01
@NotherNgineer
Copy link
Contributor

I think I would have just removed the "portapack-" from the beginning, but the important thing is that it works!

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.

None yet

4 participants