Skip to content

src cmake performs library build - #2

Open
n9wxu wants to merge 2 commits into
jmcglad:mainfrom
n9wxu:master
Open

src cmake performs library build#2
n9wxu wants to merge 2 commits into
jmcglad:mainfrom
n9wxu:master

Conversation

@n9wxu

@n9wxu n9wxu commented Mar 1, 2024

Copy link
Copy Markdown

I changed the CMakeLists.txt in the src folder so that it will successfully be incorporated into another project with add_subdirectory.

I have NOT tested this with the tests in this repo.

Comment thread src/gps.c
static char uint8_to_hex_char(const uint8_t n)
{
static const hex[] = {
static const char hex[] = {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Good catch!

Comment thread src/CMakeLists.txt
@@ -1,4 +1,6 @@
# FIXME: Add installation directives for libgps
cmake_minimum_required(VERSION 3.25)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It doesn't seem right to put cmake_minimum_required() in anything other than the top-level CMakeLists.txt file. What's the effect of cmake_minimum_required() being called multiple times in a project?

@jmcglad jmcglad left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Sorry for taking so long to get to this. Until recently, I hadn't been paying attention to my old projects on GitHub. I'd like to know more about your reasoning for the changes to src/CMakeLists.txt. I always sort of intended for the source files gps.c and gps.h to be vendored into other projects. I made that clearer in the latest update to the README. So, I'm not sure that file needs to be changed. I would like to take your change to gps.c though.

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.

2 participants