Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

C API for UptaneCycle and campaigns #1263

Merged
merged 3 commits into from
Aug 2, 2019
Merged

C API for UptaneCycle and campaigns #1263

merged 3 commits into from
Aug 2, 2019

Conversation

eu-siemann
Copy link
Contributor

No description provided.

@eu-siemann
Copy link
Contributor Author

I still need to do something with absolute paths in test C app.

@eu-siemann eu-siemann force-pushed the feat/c-api branch 2 times, most recently from 2dc475d to 1ec2ecf Compare July 23, 2019 11:52
Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

Seems reasonable!

try {
auto r = a->CampaignCheck().get();
if (!r.campaigns.empty()) {
return new Campaign(r.campaigns[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it fair to assume there will never be multiple campaigns? It would indeed be confusing if there were, but a comment to state the assumption might be worthwhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not supported at the moment, so I think it's worth keeping it simple here. I'll add a comment.

$(CC) -c -o $@ $< $(CFLAGS)

api-test: $(OBJ)
$(CC) -o $@ $^ $(CFLAGS) $(LIBS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use cmake? And either way, is there a way to integrate the test into the ctest framework?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started with having this test app completely separate, to be sure that linking works without any implicit cmake magic. But I guess I'll use cmake and add it to the ctest now.

@@ -17,9 +17,6 @@ add_aktualizr_test(NAME packagemanagerfake SOURCES packagemanagerfake_test.cc)
# Debian backend
if(BUILD_DEB)
set_property(SOURCE packagemanagerfactory.cc packagemanagerfactory_test.cc PROPERTY COMPILE_DEFINITIONS BUILD_DEB)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably don't need to set this BUILD_DEB definition here anymore, right? In fact, is BUILD_DEB meaningful at all anymore? In CI it's built by default, and if it brings in no dependencies, there's hardly a reason not to just have it always on.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a fair point, though we'd ship a code path that calls dpkg on yocto systems where it doesn't make sense. I don't know if the cost of keeping it (low, I think) warrants larger executables and potential bug surface.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably true. Perhaps we should disable by default on CI and only enable it as necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Our approach on CI has been to enable all flags, so that everything can be tested at once. The things that are not covered tend to not compile after refactors.

Also, some unrelated useful tests use the Debian mode. For example memory_test. We'll probably migrate them when we can though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. I have no objection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patrickvacek, @lbonn Sorry, I'm a bit confused, what's your conclusion on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think just keep it as is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, that's what I thought, just wanted to be sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, sorry for the distraction!

std::cerr << "Creating exception: " << e.what() << std::endl;
return NULL;
}
return a;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, there will be a need/requirement to return some status/error code to am app using the C version of libaktualizr, otherwise the only way to find out what went wrong is by parsing stderr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mike-sul Absolutely. But first, we'll need to make sure that exceptions thrown by libaktualizr are consistent and meaningful. I plan to work on that next.

@codecov-io
Copy link

codecov-io commented Jul 26, 2019

Codecov Report

Merging #1263 into master will decrease coverage by 0.65%.
The diff coverage is 1.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1263      +/-   ##
==========================================
- Coverage    79.7%   79.04%   -0.66%     
==========================================
  Files         173      175       +2     
  Lines       10301    10343      +42     
==========================================
- Hits         8210     8176      -34     
- Misses       2091     2167      +76
Impacted Files Coverage Δ
src/libaktualizr/primary/aktualizr.h 100% <ø> (ø) ⬆️
src/libaktualizr-c/test/api-test.c 0% <0%> (ø)
src/libaktualizr-c/libaktualizr-c.cc 0% <0%> (ø)
src/libaktualizr/package_manager/debianmanager.cc 100% <100%> (ø) ⬆️
src/libaktualizr/package_manager/ostreemanager.cc 76.27% <0%> (-2.97%) ⬇️
src/libaktualizr/uptane/directorrepository.cc 95.71% <0%> (-1.43%) ⬇️
src/libaktualizr/storage/sql_utils.h 83.09% <0%> (-1.41%) ⬇️
src/libaktualizr/storage/sqlstorage.cc 74.74% <0%> (-0.9%) ⬇️
src/libaktualizr/primary/sotauptaneclient.cc 91.29% <0%> (-0.62%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c8877f...4525bca. Read the comment docs.

@eu-siemann eu-siemann force-pushed the feat/c-api branch 3 times, most recently from ed8b59f to b5a0d4f Compare July 26, 2019 14:15
Eugene Smirnov added 3 commits July 26, 2019 16:59
Signed-off-by: Eugene Smirnov <[email protected]>
We shouldn't expect Config object to be alive
after returning from Aktualizr constructor.

As uptane_client_ uses Config in its destructor,
we need to move the member declaration higher.

Signed-off-by: Eugene Smirnov <[email protected]>
API of libdpkg is unstable and its usage is discouraged
for purposes other than internal.
It also prevents building libaktualizr as a shared library.
As getInstalledPackages() functionality of debian package
manager seems to be unused, we will avoid reimplementing it for now.

Signed-off-by: Eugene Smirnov <[email protected]>
void Aktualizr_destroy(Aktualizr *a);

Campaign *Aktualizr_campaign_check(Aktualizr *a);
int Aktualizr_campaign_accept(Aktualizr *a, Campaign *c);
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a bit annoying that we have to pass an Aktualizr instance to all of these. Would be handy if Campaign contained a pointer. But that might need changes in the C++ object, don't know if we want that.

Copy link
Contributor Author

@eu-siemann eu-siemann Jul 30, 2019

Choose a reason for hiding this comment

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

I see no problem in this, tbh. That's pretty much idiomatic C and is equivalent to Aktualizr->CampaignAccept(Campaign *c). If we do just a single arg here, I would expect our C++ API to also change to Campaign->Accept(). I'm not sure if it makes it nicer, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I think you're right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, maybe it's not a bad thing to make those calls members of Campaign object. This way, we would prevent passing a campaign_id that belongs to another Aktualizr instance (a hypothetical situation, but still). WDYT, @lbonn @patrickvacek @mike-sul ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it doesn't require changes to the C++ code, then that seems like the way to go. Even if it does, that seems to me to be a slightly better way to go.

err = Aktualizr_campaign_accept(a, c);
Aktualizr_campaign_free(c);
if (err) {
return EXIT_FAILURE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will the aktualizr instance be properly tear-downed and freed in this case as well as in case of line #23 ? Or it's not important taking into account that the process is going to die anyway ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't, I forgot how to write proper C ;) But I'm rewriting this piece anyway

@eu-siemann
Copy link
Contributor Author

Would you mind guys merging this in the current state? It looks like doing the proper test for that will take me a bit longer.

Copy link
Contributor

@lbonn lbonn left a comment

Choose a reason for hiding this comment

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

Even if some comments are unresolved, this looks mostly fine and dropping libdpkg is already good.

So we can probably merge it and continue to work on it in other PRs.

@eu-siemann
Copy link
Contributor Author

@lbonn Cool, I'll merge it then and address the comments about the test app in the next PR.
We also can discuss again if we want to change the Campaigns API, I'm still not sure about that.

@eu-siemann eu-siemann merged commit 4325122 into master Aug 2, 2019
@eu-siemann eu-siemann deleted the feat/c-api branch August 2, 2019 14:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants