Skip to content

pkg/oonf_api: Use MRI script to combine archives. #12155

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
Merged
merged 1 commit into from
Sep 9, 2019
Merged

pkg/oonf_api: Use MRI script to combine archives. #12155

merged 1 commit into from
Sep 9, 2019

Conversation

jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Sep 2, 2019

Contribution description

The OONF package is combining multiple ".a" file into a single archive. The way it was being done involved creating and changing directories, unpacking the original archives and repacking them into a combined one.

Theis has a couple of issues:

  • It is untidy and wasteful.
  • It breaks when thin archives are enabled, as a thin archive cannot be unpacked.

This commit uses a MRI script to do the combining step. It works both with and without thin archives and is cleaner overall.

An issue that remains to be soved is that make is calling itself to create the archive, as the PARTIAL_ARCHIVES are not known before hand. This is hacky. It can be solved but it is a subject for another PR.

Testing procedure

The OONF example should still compile.

Issues/PRs references

needed for #10195 .

@jcarrano
pkg/oonf_api: Use MRI script to combine archives.
55eccb6 
The OONF package is combining multiple ".a" file into a single archive. The
way it was being done involved creating and changing directories, unpacking
the original archives and repacking them into a combined one.

Theis has a couple of issues:

- It is untidy and wasteful.
- It breaks when thin archives are enabled, as a thin archive cannot be
  unpacked.

This commit uses a MRI script to do the combining step. It works both with
and without thin archives and is cleaner overall.

An issue that remains to be soved is that make is calling itself to create the
archive, as the PARTIAL_ARCHIVES are not known before hand. This is hacky. It
can be solved but it is a subject for another PR.
@jcarrano jcarrano added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 2, 2019
@jcarrano jcarrano mentioned this pull request Sep 2, 2019
Merged
benpicco
benpicco reviewed Sep 9, 2019
View reviewed changes
benpicco
benpicco approved these changes Sep 9, 2019
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment •
edited
Loading

Choose a reason for hiding this comment

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

tests/pkg_oonf_api still compiles and produces the same output.

But the test is basically a noop, it only does

puts("oonf_api package compiled!");

and doesn't actually call any oonf_api function :/

@benpicco
Copy link
Contributor

benpicco commented Sep 9, 2019

The tests inside bin/pkg/mlpa_tracker-v2/oonf_api/tests still work though! (after calling generate_makefiles.sh and fixing some bitrot)

@benpicco benpicco mentioned this pull request Sep 9, 2019
Merged
@jcarrano
Copy link
Contributor Author

jcarrano commented Sep 9, 2019

@benpicco

bitrot

Mmm, how bit-rotted was it? Shall we bring the sledgehammer and demolish oonf_api?

@benpicco
Copy link
Contributor

benpicco commented Sep 9, 2019

@jcarrano Looks like you are on a killing spree 😄

The oonf_api library contains it's own suite of tests. When I ported it to RIOT many years ago, I also added Makefiles to build the tests on RIOT (and tweaked some buffers to fit onto the trusty msb-a2).

Turns out those Makefiles were never used, so when RIOT changed around them, they remained static, unaware of path changes and new CFLAGS.

I'm actually surprised it took so little to get them working again.

Now for users, I don't know. The nhdp module uses it, but that's on it's way out.
I'm not using it either. But we don't have upstream users for most of pkg/, it's more of a toolbox for downstream users.

@jcarrano
Copy link
Contributor Author

jcarrano commented Sep 9, 2019

The oonf_api library contains it's own suite of tests.

oh, ok, that changes everything, especially if you got them working away. Anyways, it was scary to find out that the tests were not working.

@benpicco
Copy link
Contributor

benpicco commented Sep 9, 2019

I got them working, but not in a CI-able way, that is another task.

@benpicco benpicco merged commit 4e4414d into RIOT-OS:master Sep 9, 2019
@benpicco
Copy link
Contributor

It is true though that this pkg still points to oonf_api 0.3.0 from 2013.
The latest upstream version is 0.15.1 and is of much more recent vintage.

So interest in this package might indeed not be very high anymore.

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@jia200x jia200x Awaiting requested review from jia200x

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
3 participants
@jcarrano @benpicco @kb2ma