https://bugzilla.redhat.com/show_bug.cgi?id=1264546 --- Comment #36 from Gustavo Lima Chaves <gustavo.lima.chaves@xxxxxxxxx> --- > Hi Gustavo, Hi, Paulo. > > I started the process to become your sponsor. > > I believe you showed that you will have enough persistence > to maintain the package, and add new packages to Fedora :) Thank you so much! Yes, I'll do my best at all times, don't worry. > > You should update the changelog as if the package were already > in Fedora. Ok, from now on, I'll do that. > > I will review the new package, and let you know if anything > else is required before the first official Fedora package :) > > The first issue I see in a quick look is that you did > > %package %{name}-subpackage > > it should have been > > %package subpackage > > now you have packages named: > > soletta-soletta-subpackage > > When not using -n, "%{name}-" is prepended to the package > name. Mmm, fine. I'll fix it. > > Hi Gustavo, > > ---%<--- > First we will need to sort out some issues with the third party > software. > > I see there is a review request for tinycbor at > https://bugzilla.redhat.com/show_bug.cgi?id=1269001 > It is the same thing right? Yes, it is. As soon as they get in, we can change to depend and link to it. > > About duktape I do not see any review or existing package. > > Could tinycbor and/or duktape be used as external dependencies? > If not, what is the reason? Duktape specifically can't, as they only distribute the sources at will, to be bundled by user projects. > > Note that Fedora is in the process of having more relaxed rules > for bundled packages, but it should only be used if there is no > other option. > > > ---%<--- > Another issue I noticed, is that %install appears to redo all > compilations, after regenerating header files. Some issue with > make rules? Yes, some issue on our side, it's on my TODO-list. > > > ---%<--- > There are some duplicated files. I think you misunderstood some > details of adding a trailing "/". If listing only the directory, > must do it as: > > %dir %{_libdir}/soletta/modules/ > > otherwise, there will be duplicates, because when listing a > directory it understands the directory and its contents, for > example: > > %{_libdir}/soletta/modules/ > %{_libdir}/soletta/modules/flow/ > > the line %{_libdir}/soletta/modules/ already includes > %{_libdir}/soletta/modules/flow/ > > Another issue is the line: > > %{_includedir}/soletta/* > > it should have been: > > %{_includedir}/soletta/ > > otherwise, the directory %{_includedir}/soletta/ will not > have a owner package. Sorry for the mistakes, I'll have those fixed. > > > ---8<--- > Please comment about the warning: > > In function '__fread_alias', > inlined from 'sol_memmap_write_raw_do.isra.2' at ./src/lib/io/sol-memmap-storage.c:169:15: > /usr/include/bits/stdio2.h:290:9: warning: call to '__fread_chk_warn' declared with attribute warning: fread called with bigger size * nmemb than length of destination buffer > return __fread_chk (__ptr, __bos0 (__ptr), __size, __n, __stream); > > It is caused by the fread in this code: > if (mask) { > uint64_t value = 0, old_value; > uint32_t i, j; > > for (i = 0, j = 0; i < entry->size; i++, j += 8) > value |= (uint64_t)((uint8_t *)buffer->data)[i] << j; > > ret = fread(&old_value, entry->size, 1, file); > > It looks suspicious. Shouldn't old_value be declared > as size_t, and instead of entry->size, write sizeof(old_value) > or sizeof(size_t) ? If the size is guaranteed to fit, it will > have issues in big endian architectures. I saw it too, it won't be there anymore on the next beta release. Thanks. > > > ---8<--- > Also fix the %package %{name}- issue. > > Will do. > ---8<--- > For the next review, please also update the changelog. This also > serves as a way to follow the history of the package :) Okay. It may take me a few time for the next update, since I want to sort that arm bot error first. I guess I'll try it on a raspberry-pi fedora image here, unless I can get access to some of the community arm machines :/ Thanks a lot for the review and sponsoring again :) -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review