Hi Will, this is mostly fine, a few hints to do now or later and a few questions that are either changes to the upload or explanations. If you go for any changes before the re-review please mention them in the changelog (and see #3 for even more).
-- #1 (non-blocking - no action needed) let us start with a topic you know from the review for emhash (LP: 2130190) - file-exclude. In this case all third-party seem to be declared (probably since this started from an accepted debian package). There is one minor catch - that is: third_party/ngen/COPYRIGHT: MIT License But weirdly all the files in there are Apache-2 as specified in the copyright file. So I assume more that third_party/ngen/COPYRIGHT is wrong and debian/copyright is correct. #2 (non-blocking - improve on next upload) In terms of namespace, thanks you for explaining the -sycl naming. Same for the explanation that it is the same but with modifications and why. It is nice to have it here in the bug, but to help future packagers even more it would be great to have a README.source (https://lwn.net/Articles/280471/) that explains all this in place. #3 (non-blocking - improve on next upload) To reflect that it has all that history of the onednn package it is derived from I think (only 90% sure without testing) that you could and should keep the old entries in the changelog. IMHO it is fine and even helpful to have this to be like: onednn-sycl - v3 onednn - v2 onednn - v1 Otherwise it appears so much out of no-where until people found this bug and read through it. #4 (please fix or explain) You did add the sycl name to separate the binary packages, and they have shared paths on disk as users need to decide which they want. I understand that so far, I'm unhappy that this is the split over runtime-code selection, but no one in any upstream seems to have the patience anymore to to it right *sigh* (not your fault). And we see -rocm, -sycl, ... :-/ there is a limit to how much that can be tolerated. We do not yet cross that line but it can't just proliferate more. On one hand we need to see that these split out packages for dedicated use are not just "added and gone" but "maintained well over all the releases into the future". All that is only a call to the responsibility, what I'd like to ask for or get changed is the handling of bin:onednn-examples. You namespaced all else, why not this. I know it does not exist in the src:onednn but if they add it as well one day that would be the name. So a collision has a high chance to happen. This is a bit of a circular dependency. The Debian Deep Learning Team does not need to know or care about src:ondnn-sycl so if they introduce the same they are fine. But we then suddenly have a name collision and on top the lack of a breaks replaces. There is no sweet "this will fix it all" outcome of this :-/ My gut feeling tells me the following might be the path of least breakage along the way 1. contribute a PR to add onednn-examples to src:onednn in Debian 2. make yours break/conflict with the one introduced in Debian 3. make yours have the -sycl namespace in the name please Depending on the reaction to #1 the actions #2/#3 need to be adapted, maybe they say "no and never" but then at least we know. #5 (please fix or explain) I wondered about the soname in the package name - libdnnl-sycl3. src:onednn has libdnnl3.6 - which is a more fine grained version. Don't you fear you have to transition from 3.10 to 3.11 some day? I mean it can work just fine, but a hint why you have chosen to differ there would be great. Furthermore the symbols file is a total mess, all only c++ generates symbols - the src:onednn has this much nicer with a few C++ filters and the rest proper symbols. And https://github.com/uxlfoundation/oneDNN/releases/tag/v3.10 does not indicate that they changed this a lot, so should it not be able to be similar? C++ symbols are sadly mostly noise. But at the same time https://github.com/uxlfoundation/oneDNN/releases/tag/v3.10 suggests that sometimes functions are dropped (there is a deprecation). Tasks: - Could you please go for a symbols file similar to the one of src:onednn to help automatic version detection of dependencies - Could you please explain the choice for just libdnnl-sycl3 ? P.S. For any of my requests that are "(please fix or explain)" that end up via "explanation" please also add that knowledge to the README.source I asked for. ** Changed in: Ubuntu Resolute Status: In Progress => Incomplete -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/2136925 Title: [needs-packaging] onednn-sycl (built with DPC++ toolchain) To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+bug/2136925/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
