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.

Sign up for GitHub

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

Jump to bottom

CMakeLists for utils, platform, and rtos targets #103

Merged
merged 1 commit into from
Jul 24, 2024
Merged

CMakeLists for utils, platform, and rtos targets #103

merged 1 commit into from
Jul 24, 2024

Conversation

nicka101
Copy link
Contributor

  • Adds CMakeLists.txt for most of the targets in the utils, platform and rtos folders.
  • Enables ASM in the project (required by platform_cmsis, and util_crash_catcher)
  • Add CMake to the docker image and updates devcontainer.json to match current standards

In its current state, I don't attempt to cover the options/defines from the Makefiles (primarily just the targets themselves and their includes/links), and platform_cmsis seems to be the biggest blocker to compilation, representing the vast majority of the errors. The fact it doesn't build successfully is also the primary reason this PR is marked draft, though neither does the HEAD of the cmake branch, so feel free to merge this if you want

@shymega
Copy link
Collaborator

shymega commented May 23, 2024

I'll review this this weekend. What I'll likely do is merge into cmake, and we can keep working on that branch. If it's better for you, and I could grant you access to my fork on that branch.

@Ralim I will handle this PR =)

@nicka101
Copy link
Contributor Author

I'll review this this weekend

Cool, no rush. I mostly opened this so we can avoid redoing the same work separately

What I'll likely do is merge into cmake, and we can keep working on that branch

Makes sense, and is the target branch for this PR anyway :)

If it's better for you, and I could grant you access to my fork on that branch.

Up to you, I don't mind working via PR, but it might be easier with push access to cmake. If you've got a preferred platform outside of Github for coordinating our efforts, let me know

shymega
shymega requested changes May 25, 2024
View reviewed changes
Copy link
Collaborator

@shymega shymega left a comment

Choose a reason for hiding this comment

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

Overall, I think we can merge, but I just wanted to delve into these lines.

Thanks.

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
src/platform/CMakeLists.txt Show resolved Hide resolved
src/utils/CMakeLists.txt Show resolved Hide resolved
@shymega
Copy link
Collaborator

shymega commented Jul 19, 2024

Sorry for the delay in review. I've got round to it now.

@shymega shymega marked this pull request as ready for review July 19, 2024 18:54
@shymega shymega requested a review from Ralim July 19, 2024 19:01
@shymega
Copy link
Collaborator

shymega commented Jul 19, 2024

@Ralim I've requested your review here - would like to get your clearance on this PR.

@Ralim
Copy link
Collaborator

Ralim commented Jul 20, 2024

@shymega cmake isn't my forte so happy to leave primary review to you.

@shymega
Copy link
Collaborator

shymega commented Jul 21, 2024

@Ralim Thanks, will do.

@shymega
Copy link
Collaborator

shymega commented Jul 21, 2024

@nicka101 I've rebased your branch, and applied the fixes. I'm keen to get this into my branch as soon as possible, and I'm especially grateful for your work here.

Would you agree that squashing the commits in this PR would be a good idea?

Cheers.

@nicka101
Copy link
Contributor Author

By all means, do whatever you see fit to manage the branch.
I haven't had much time to work on this lately, hopefully I'll have some more soon, but I can always pull/rebase as needed

@nicka101 @shymega
WIP CMakeLists for utils, platform, rtos
53f1df8 
Main work by @nicka101.

Also, by @shymega:

- Add another `target_link_libraries` to util_sha256
    This links to CMSIS and HAL libs.
- Fixup Dockerfile for GCC ARM download, and tidy
- Had to run `dos2unix` on some files.
- Remove `*.cmake` from .gitignore
  This allows us to keep future CMake modules, for example, Corrosion.
- Update devcontainer.json name
- Move `.devcontainer/devcontainer.json` to root of repo

Signed-off-by: Dom Rodriguez <[email protected]>
@shymega
Copy link
Collaborator

shymega commented Jul 24, 2024

@nicka101 Alright. I'll merge this now, so we can progress with the CMake conversion.

Many thanks for your contribution! Anything else, feel free to open more PRs - we could sure use it!

@shymega shymega merged commit 61505fb into pine64:cmake Jul 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shymega shymega shymega requested changes

@Ralim Ralim Ralim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicka101 @shymega @Ralim