Skip to content

pkg: udpate ccn-lite version to include patches for RIOT#6364

Closed
smlng wants to merge 2 commits intoRIOT-OS:masterfrom
smlng:pr/pkg/update_ccn_lite
Closed

pkg: udpate ccn-lite version to include patches for RIOT#6364
smlng wants to merge 2 commits intoRIOT-OS:masterfrom
smlng:pr/pkg/update_ccn_lite

Conversation

@smlng
Copy link
Copy Markdown
Member

@smlng smlng commented Jan 15, 2017

replaces #6352 to bump ccn-lite package version as soon as their PR 100 is merged

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 15, 2017

is needed by #6341

@smlng smlng added Area: pkg Area: External package ports Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jan 15, 2017
@smlng smlng added this to the Release 2017.01 milestone Jan 15, 2017
This was referenced Jan 15, 2017
Closed
Closed
@smlng smlng added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 15, 2017
@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Jan 15, 2017

cn-uofbasel/ccn-lite#100 is merged.

@smlng smlng force-pushed the pr/pkg/update_ccn_lite branch from 13b846c to 2b258e2 Compare January 15, 2017 10:16
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 15, 2017

fixed version to latest head (including merge commit) of ccn-lite. Jenkins was already fine with the version, lets give Murdock a try as well.

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jan 15, 2017
OlegHahm
OlegHahm approved these changes Jan 15, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@OlegHahm OlegHahm left a comment

Choose a reason for hiding this comment

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

ACK

@smlng smlng force-pushed the pr/pkg/update_ccn_lite branch 2 times, most recently from c8a44fd to 02c71af Compare January 15, 2017 10:20
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 15, 2017 •
edited
Loading

@OlegHahm had to remove not required patch to make it work - Jenkins found it. Anyhow, Murdock wasn't done yet, so lets wait for it.

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Jan 15, 2017

Hm, apparently there is more to be done.

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 15, 2017

but macOS works 😜 anyhow, I'll see to it - more PRs at ccn-lite, then

cgundogan
cgundogan reviewed Jan 15, 2017
View reviewed changes
pkg/ccn-lite/patches/0001-fix-compiler-warnings.patch Outdated
for (pi = i->pending; pi; pi = pi->next) { // check whether already listed
if (pi->face == from) {
DEBUGMSG_CORE(DEBUG, " we found a matching interest, updating time\n");
- pi->last_used = CCNL_NOW();
Copy link
Copy Markdown
Member

@cgundogan cgundogan Jan 15, 2017

Choose a reason for hiding this comment

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

I thought we ignore whitespace errors of external packages?

Copy link
Copy Markdown
Member Author

@smlng smlng Jan 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

was my editor that deleted them automatically ...

@smlng smlng force-pushed the pr/pkg/update_ccn_lite branch 2 times, most recently from afdaaf7 to f28b942 Compare January 15, 2017 17:16
smlng
smlng commented Jan 15, 2017
View reviewed changes
pkg/ccn-lite/patches/0001-fix-compiler-warnings.patch Outdated
DEBUGMSG_CFWD(DEBUG, " content not added to cache\n");
free_content(c);
@@ -201,7 +201,7 @@ ccnl_fwd_handleInterest(struct ccnl_relay_s *relay, struct ccnl_face_s *from,
int nonce = 0;
Copy link
Copy Markdown
Member Author

@smlng smlng Jan 15, 2017

Choose a reason for hiding this comment

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

I think this better should be int32_t ...

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 15, 2017

added a new patch to get rid of those compiler errors on linux and ARM none eabi platforms. If these succeed I may open a similar PR directly at ccn-lite.

OlegHahm
OlegHahm reviewed Jan 15, 2017
View reviewed changes
pkg/ccn-lite/patches/0001-fix-compiler-warnings.patch Outdated
if (pkt != NULL && (*pkt) != NULL && (*pkt)->s.ndntlv.nonce != NULL) {
if ((*pkt)->s.ndntlv.nonce->datalen == 4) {
- nonce = *((int*)(*pkt)->s.ndntlv.nonce->data);
+ memcpy(&nonce, (*pkt)->s.ndntlv.nonce->data, 4);
Copy link
Copy Markdown
Member

@OlegHahm OlegHahm Jan 15, 2017

Choose a reason for hiding this comment

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

Why memcpy here (and below)?

Copy link
Copy Markdown
Member Author

@smlng smlng Jan 16, 2017

Choose a reason for hiding this comment

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

the original code causes a strict-alignment error by the compiler. memcpy is one possible fix, another would be to change the struct behind this <- hence, I found using memcpy less intrusive.

In general, it seems rather opportunistic to assume an int has size 4 on any platform, or not? So additionally, it might be better to define int32_t nounce = 0 as I suggested above.

Copy link
Copy Markdown
Member

@OlegHahm OlegHahm Jan 16, 2017

Choose a reason for hiding this comment

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

Agreed

@smlng smlng force-pushed the pr/pkg/update_ccn_lite branch from f28b942 to 427fabf Compare January 18, 2017 19:48
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 18, 2017

the patch added does the same as PR 102 for ccn-lite, if that gets merged, I would remove the patch here and update the package version accordingly: @OlegHahm or @cgundogan someone feels lucky to merge the referenced PR for ccn-lite?

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Jan 18, 2017

Too late. ;)

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 18, 2017

@OlegHahm yes, but still problem see my comment in #6399 and IRC

@smlng smlng force-pushed the pr/pkg/update_ccn_lite branch from 427fabf to 02e37d3 Compare January 18, 2017 20:27
@smlng smlng force-pushed the pr/pkg/update_ccn_lite branch from 02e37d3 to 738f754 Compare January 18, 2017 20:44
@smlng smlng added the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 18, 2017
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 18, 2017

depends on #6399

@smlng smlng mentioned this pull request Jan 19, 2017
Merged
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 19, 2017

obsoleted by #6399

@smlng smlng closed this Jan 19, 2017
@smlng smlng deleted the pr/pkg/update_ccn_lite branch January 27, 2017 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgundogan cgundogan cgundogan left review comments

@OlegHahm OlegHahm OlegHahm approved these changes

Assignees

@cgundogan cgundogan

@OlegHahm OlegHahm

Labels

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 State: waiting for other PR State: The PR requires another PR to be merged first Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@smlng @OlegHahm @cgundogan