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.

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

Split Ftp plugin into Ftp and FtpServer #2060

Merged
merged 18 commits into from Nov 2, 2023
Merged

Split Ftp plugin into Ftp and FtpServer #2060

merged 18 commits into from Nov 2, 2023

Conversation

julianoes
Copy link
Collaborator

@julianoes julianoes commented May 29, 2023

The FTP plugin is a weird combination of client and server in one. Now that we have Components additional to Systems we can split it into client and server.

This includes:

  • Complete refactor (almost rewrite) of server and client
  • Slight API changes
  • Using std::filesystem as much as possible everywhere
  • Adding system tests

Depended on mavlink/MAVSDK-Proto#319.

Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Nitpick: the CI/Fedora stuff could have been done in another PR, this one is already big enough 🙈

@julianoes
Copy link
Collaborator Author

Oh, right. I was going to pull these commits out when I clean it up.

@julianoes
Copy link
Collaborator Author

I'm still hunting a crash in CI that I can't replicate...

@julianoes julianoes force-pushed the pr-split-ftp branch 11 times, most recently from b41f172 to 488e6a5 Compare August 28, 2023 01:16
@julianoes julianoes force-pushed the pr-split-ftp branch 3 times, most recently from 7224a85 to f964fa1 Compare August 29, 2023 04:45
@julianoes julianoes changed the title [WIP] Split Ftp plugin into Ftp and FtpServer Split Ftp plugin into Ftp and FtpServer Oct 6, 2023
@julianoes julianoes marked this pull request as ready for review October 6, 2023 19:54
@julianoes julianoes force-pushed the pr-split-ftp branch 2 times, most recently from 71bae31 to fbbe947 Compare October 24, 2023 08:25
This includes:
- Refactor of server and client
- Slight API changes
- Using std::filesystem as much as possible everywhere.
- Adding system tests

Signed-off-by: Julian Oes <julian@oes.ch>
This is to get more information about segfaults in CI.

Signed-off-by: Julian Oes <julian@oes.ch>
No need for 2 separate runners.

Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
It turns out stack smashing seems to happen in create_temp_file, for
whatever reason. This is a very desperate attempt to try to avoid it.

Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
@julianoes julianoes force-pushed the pr-split-ftp branch 4 times, most recently from 25f8f98 to a890302 Compare November 2, 2023 03:25
Somehow these often segfault. I've spent days on this, and have to
give up.

Signed-off-by: Julian Oes <julian@oes.ch>
JonasVautherin
JonasVautherin previously approved these changes Nov 2, 2023
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

That was a big effort! Congrats 😊

It now depends on the incoming FTP messages.

Signed-off-by: Julian Oes <julian@oes.ch>
QGC requests paths with a leading /. If we just remove it, that works
inside of the defined root path.

Signed-off-by: Julian Oes <julian@oes.ch>
@julianoes julianoes merged commit 578ec76 into main Nov 2, 2023
27 checks passed
@julianoes julianoes deleted the pr-split-ftp branch November 2, 2023 23:05
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.

None yet

3 participants