-
-
Notifications
You must be signed in to change notification settings - Fork 510
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
Support to generate .deb packages #130
Conversation
1a57149
to
9ef889f
Compare
provider: releases | ||
skip_cleanup: true | ||
api_key: | ||
secure: hBX3pFWNZiDbz4yKnOjhLg3QS9Ubn1XePxSeIt2Btq5GzbomOPDCgpIFijBppliwj9oKc302EMnZSg2QWeAzFKn9UnmIflJ0E4iymYgwWdTJv+bSnYALJEmO8F6gF9FgRlPk8FCtZiECoTsa75w5TrEZKZpFpmzVYRiDu0eo6sEjW7UJPC0A2KSTXLrBCHSIZy/iasbGmuur4brG7NO0QdMOvDXvhsYfkXDRJFMTtTHvLiKJcqiunPfqARzf1H4x4iczRYscKu5Vn8Kmw3NANGkcIDvEj4ooih831EXxACRZw0VgycgNHOKRXKC9pZ4hLQMon+jxpQX+X8k/K5161oEkF/gCVKyFb31Pk/4Uwe81p1GJY2lAC7MDUxA98RKXhdvVYF2Cp44+IbF0YVoWRUtVAhknXRQ3Weg25kyVSu83q2nN2nZq2qGTnpNIbdN56s/F+uaFtipGEh+vmiv8rNUz+Z5MFrY2FQaSvBTFw9K4tNs9uc+VQd1bE7X5wh0yywEqUEw2nzqTB2xR+OubygUASbk2GLNdc254P0lrzCHbNM62Y7sRX06CM7hPlwhELEkVtUXZWJ0KuhQyLvRh3aPJ3Jj30EswTt/FGT1gzSP1FjjHBRZCK4P2D2rwJ5TMn2JrZKfPxmEd3kVmn6h80+gBbKgonGmZspd2SvPEI5g= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a stupid question, but if this key is secure, why is it public here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only see the encrypted string. It's a common way with travis.
https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml
But I appreciate that you check things like that!
add_executable(fly_mission | ||
fly_mission.cpp | ||
) | ||
|
||
# Not needed if DroneCore installed system-wide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the fact this is removed mean that you have sorted out Windows case for installing system wide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have currently no access to a Windows machine. I'll come back to the Windows build once I do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the plan is to do that before this is merged? It doesn't "matter" except that the example docs will need to be changed to add "not supported" for windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore that, I see below that you're hopefully going to do that as part of this PR :-)
endif() | ||
|
||
# Add DEBUG define for Debug target | ||
set(CMAKE_CXX_FLAGS_DEBUG "-DDEBUG") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release is built by default. Is this removed because we're happy for release build to be the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not actually about the default. All this does is it adds a #define DEBUG
in case it is a debug build. This is not actually used or needed in the examples, so I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification.
set(CMAKE_CXX_FLAGS_DEBUG "-DDEBUG") | ||
|
||
# This finds thread libs on Linux, Mac, and Windows. | ||
find_package(Threads REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this isn't needed because now the underlying dronecore library links to the thread library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly.
So looks good to me, but I haven't actually run it. Main questions are:
|
I'll figure that out in the next days, probably as part of this PR even.
I think we can document that you can set the prefix where to install it. I'd leave it to devs to figure out how to link to it for that case.
|
Sounds good. We can provide an example in docs.
OK. Up to you. IMO having script is "nice to have", but hardly a priority. Worth adding an issue so it can be addressed at some later point. |
I'm good with this once Windows stuff addressed (by which I mean "fixed or explicitly not supported"), but leave @mrpollo to review. |
@hamishwillee ok agreed. |
5125303
to
a3ba415
Compare
a3ba415
to
507054d
Compare
219db58
to
9fca987
Compare
9fca987
to
36b4505
Compare
This adds arguments for the Python script to set the input and output file paths. This enables to the script to be run a bit more flexible.
This moves the default install path to /usr/local. This is closer to the way that most projects are set up and alows for much easier examples.
We only need to link to libdronecore now because it is installed system-wide and the library is already linked to pthread.
libdronecore now already links to pthread.
This adds a script which can create a .deb package using checkinstall.
36b4505
to
bc3f32f
Compare
This is a step towards integration tests for github.com/PX4/Firmware by generating .deb packages on releases.
In order to streamline the .deb generation, I've changed the build to a shared library for Linux and macOS, and made the default install location
/usr/local
.While doing that I actually realized that libdronecore was not being linked to curl and pthread which required all executable to link against them. This should now be fixed.