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

reduce required modules #8139

Closed
wants to merge 7 commits into from
Closed

reduce required modules #8139

wants to merge 7 commits into from

Conversation

dagar
Copy link
Member

@dagar dagar commented Oct 16, 2017

No description provided.

@mcharleb
Copy link
Contributor

I noticed you removed the include_directories from the module CMakeLists.txt files. Are these include directories now somehow auto populated by dependencies?

@dagar
Copy link
Member Author

dagar commented Oct 17, 2017

It's poorly named, but if you use target_link_libraries it adds the dependency, links, and can inherit things like include directories, compile options, etc if they're defined PUBLIC.

Using that as much as possible could really help enforce modularity.

mcharleb
mcharleb reviewed Oct 17, 2017
View reviewed changes
src/systemcmds/ver/CMakeLists.txt
)
# vim: set noet ft=cmake fenc=utf-8 ff=unix :
version
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs newline

src/platforms/posix/drivers/df_bebop_bus_wrapper/CMakeLists.txt
)
# vim: set noet ft=cmake fenc=utf-8 ff=unix :
mixer
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs newline

src/platforms/posix/drivers/df_ak8963_wrapper/CMakeLists.txt
df_driver_framework
df_ak8963
)
# vim: set noet ft=cmake fenc=utf-8 ff=unix :
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs newline

Tools/check_submodules.sh
@@ -34,19 +34,19 @@ then
echo "Continuing build with manually overridden submodule.."
elif [ "$user_cmd" == "u" ]
then
git submodule sync --recursive
git submodule update --init --recursive
git submodule sync --recursive -- $1
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the "--" before $1 do?

Copy link
Member Author

Choose a reason for hiding this comment

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

For specifying a single submodule.

src/drivers/device/CMakeLists.txt
px4_add_library(drivers_device ${SRCS})
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs newline

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we need newlines at the end of a cmake file?

src/lib/controllib/CMakeLists.txt
)
# vim: set noet ft=cmake fenc=utf-8 ff=unix :
add_dependencies(controllib prebuild_targets)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs newline

src/lib/conversion/CMakeLists.txt
# vim: set noet ft=cmake fenc=utf-8 ff=unix :

add_library(rotation EXCLUDE_FROM_ALL rotation.cpp)
add_dependencies(rotation prebuild_targets)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs newline

Copy link
Contributor

Choose a reason for hiding this comment

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

And so do several other files below

src/modules/gnd_pos_control/CMakeLists.txt
SRCS
GroundRoverPositionControl.cpp
DEPENDS
platforms__common
external_lgpl
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to aggregate the licenses of the files used in the final build so the user can comply with all the license requirements. In this case BSD + LGPL

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes if it comes to that, but we should push to get rid of these 600 lines of LGPL code.

Copy link
Member

Choose a reason for hiding this comment

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

Paul has done a clean version that is almost ready to go.

src/modules/logger/CMakeLists.txt
@@ -34,7 +34,6 @@
px4_add_module(
MODULE modules__logger
MAIN logger
PRIORITY "SCHED_PRIORITY_MAX-30"
Copy link
Contributor

Choose a reason for hiding this comment

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

Task prio no longer needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

That priority only applies to the main.

src/modules/systemlib/CMakeLists.txt
flashparams/flashparams.c
flashparams/flashfs.c
)
elseif ("${CONFIG_SHMEM}" STREQUAL "1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure CONFIG_SHMEM check isn't needed? I think its used for muorb synchronization?

Copy link
Member Author

Choose a reason for hiding this comment

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

CONFIG_SHMEM is used within param, I'm not sure why it was left behind here.

mcharleb
mcharleb previously approved these changes Oct 17, 2017
View reviewed changes
@dagar dagar mentioned this pull request Oct 17, 2017
Merged
@dagar dagar mentioned this pull request Nov 27, 2017
Closed
@dagar
Copy link
Member Author

dagar commented Nov 27, 2017

Continued in #8378

@dagar dagar closed this Nov 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcharleb mcharleb mcharleb left review comments

@LorenzMeier LorenzMeier Awaiting requested review from LorenzMeier

@davids5 davids5 Awaiting requested review from davids5

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@dagar @mcharleb @LorenzMeier