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

Reorganize deps #724

Merged
merged 87 commits into from
May 30, 2019
Merged

Reorganize deps #724

merged 87 commits into from
May 30, 2019

Conversation

JonasVautherin
Copy link
Collaborator

@JonasVautherin JonasVautherin commented Apr 17, 2019 •
edited
Loading

This is moving the dependencies out of the project and adding them using find_package instead. Because we still want to provide a way to compile the dependencies from sources, it is organized as follows:

- superbuild
    |
    --- third_party
    |
    --- src

The superbuild adds both. Note that third_party builds the dependencies at configure time.

To be done before merging this PR:

  • Fix all CI scripts
  • Add build for iOS
  • Update documentation (by adding a Makefile showing how to use CMake)
  • Test iOS build with Swift SDK
  • Add build for Android
  • Test Android build

Resolves #718, resolves #703

Schneebeli Adrian and others added 18 commits April 8, 2019 10:54
@JonasVautherin
1. adds _acceptance_radius member variable to MissionItemImpl class
9f9fc96 
2. adds functions to set and get the acceptance radius
3. changes the get_mavlink_param2() function to return the acceptance radius if it is set,
   if it is not, it will return the predefined values (3 or 1 meter) depending on the _fly_through boolean, as it was before
@JonasVautherin @ad-snow
Apply suggestions from code review
b1d4738 
adds units as requested by JonasVautherin

Co-Authored-By: ad-snow <32896677+ad-snow@users.noreply.github.com>
@JonasVautherin
adds units to new functions and variables
31ca8ad
@JonasVautherin
fix extra whitespaces
3c47559
@JonasVautherin
using std::isfinit(), updates documentation, (adressing comments on m…
48f337f 
…r by @julianoes)
@JonasVautherin
fix style
3da16dc
@JonasVautherin
backend: disable build_external
82d6e60
@JonasVautherin
core: do not build dependencies
f2db8ff
@JonasVautherin
backend: build with external deps instead of submodules
f3d8846
@JonasVautherin
backend: clean up, use BUILD_SHARED_LIBS, remove old comments
94a479a
@JonasVautherin
reorganize files into src/ and tools/
1dde59f
@JonasVautherin
build dependencies as SUPERBUILD
9241705
@JonasVautherin
remove curl https tests as we compile without https support
e369586
@JonasVautherin
cmake: minor style fixes
eec1457
@JonasVautherin
fix travis gcc build
be9d403
@JonasVautherin
remove third_party submodules
b627272
@JonasVautherin
fixup! fix travis gcc build
002142d
@JonasVautherin
enable position independent code by default
df1426b
@JonasVautherin JonasVautherin force-pushed the reorganize-deps branch 2 times, most recently from 71c03db to ab77032 Compare April 18, 2019 12:02
@JonasVautherin JonasVautherin force-pushed the reorganize-deps branch 2 times, most recently from 5895fb3 to 2fca26e Compare April 18, 2019 20:52
julianoes
julianoes requested changes May 27, 2019
View reviewed changes
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

This is great overall. I made some small comments.

And the Makefile to teach users about the new way is missing.

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
example/battery/CMakeLists.txt Show resolved Hide resolved
example/calibrate/CMakeLists.txt Show resolved Hide resolved
src/backend/src/backend_api.h Show resolved Hide resolved
src/backend/src/grpc_server.h Outdated
#include "param/param_service_impl.h"
#include "plugins/offboard/offboard.h"
#include "offboard/offboard.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I agree with this change? What was your thought?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it "just happened" when I moved files (before we had something like plugins/offboard/include/offboard/offboard.h that I removed, and possibly it comes from there).

I can move it back to plugins/offboard/offboard.h if you want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking into it, I changed it because I did not feel like including all of ${PROJECT_SOURCE_DIR} (here. In order to be able to call #include "plugins/..." I need to add the directory containing plugins to the target_include_directories, which would mean that I would include e.g. integration_tests/ there, right? What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can revert back to having the header in e.g. src/plugins/action/include/plugins/action/action.h. I just thought to remember that at first you were not really in favor of that.

Your call here, I'm personally not sure what's best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well actually it's maybe better to have the include/ folder in order to declare that correctly when exporting the headers. I'll change that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand. What was wrong with how it was?
It was introduced here and I think made sense: #435

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I just remembered you did not like it at first and thought it was not necessary anymore, but actually I believe it made more sense.

src/integration_tests/mission_survey.cpp Outdated Show resolved Hide resolved
src/plugins/action/action.h Outdated Show resolved Hide resolved
tools/create_packages.sh Outdated Show resolved Hide resolved
tools/create_packages.sh Outdated Show resolved Hide resolved
@julianoes julianoes mentioned this pull request May 27, 2019
Merged
@JonasVautherin JonasVautherin force-pushed the reorganize-deps branch 2 times, most recently from 4dc08c6 to 88c0ceb Compare May 29, 2019 07:50
@JonasVautherin JonasVautherin marked this pull request as ready for review May 29, 2019 13:41
julianoes
julianoes previously approved these changes May 29, 2019
View reviewed changes
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

I scrolled through this again and it looks good!

Makefile
@echo ""
@echo "You probably want to run something like:"
@echo ""
@echo " ./build/default/integration_tests/integration_tests_runner"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

src/backend/src/CMakeLists.txt
@@ -62,7 +62,7 @@ if(IOS)
BUILD_WITH_INSTALL_RPATH TRUE
INSTALL_NAME_DIR @rpath
PUBLIC_HEADER backend_api.h
XCODE_ATTRIBUTE_CODE_SIGN_IDENTITY "Dronecode SDK"
#XCODE_ATTRIBUTE_CODE_SIGN_IDENTITY "iPhone Developer"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This guy 😄!

julianoes
julianoes previously approved these changes May 29, 2019
View reviewed changes
@JonasVautherin JonasVautherin mentioned this pull request May 29, 2019
Merged
julianoes
julianoes reviewed May 30, 2019
View reviewed changes
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Hurray let's get this in!

@julianoes julianoes merged commit 4c8b1c1 into develop May 30, 2019
@julianoes julianoes deleted the reorganize-deps branch May 30, 2019 07:12
@JonasVautherin JonasVautherin mentioned this pull request Jun 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianoes julianoes julianoes 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.

Dronecode SDK Windows build binary for Python package Automate dependencies for Linux and Windows
2 participants
@JonasVautherin @julianoes