Add mesa snapshot overlay #72
Conversation
|
I gave this a cursory read to review the general approach, and it's as we discussed. I'm not super happy that we duplicate packaging in this repo and that we allow for calling random scripts, but that works in the short-term. @basak-qcom Would you be able to do a full review? Or I can do one on Monday |
Test jobs for commit e5f2734 |
Test jobs for commit 7ea28c1 |
There was a problem hiding this comment.
[Comment only] Ideally we'd have a better mechanism to do all of this, such as packaging repositories in git that can derive from upstream and Debian packaging commits. But in the meantime to make progress we already have overlay-debs debdiffs as a compromise. Adding a generic script mechanism seems like a clean way of approaching this. Even though it's more creep towards the less ideal, I can't think of a better general approach.
[No change needed] We have a service dependency on gitlab.freedesktop.org being introduced here, whereas before we depended on Debian apt archives only I think. Again, given this is a stop-gap solution, I guess that's OK. Perhaps we should start noting these somewhere though.
[For discussion before landing] The debian/ directory and packaging inside gives me some concern. It's being copied from Debian experimental I think, and then being modified? Since our goal is to minimise any "delta" to upstream over time, ideally we'd store the diff rather than a copy. Notably this approach is the one we take with the overlay-debs debdiff mechanism, which is what this is building upon. I wouldn't do that for the entire upstream snapshot, but perhaps we can do that for the packaging, supplying a debdiff to apply over the source package once it has been constructed from the upstream git commit and the debian directory from Debian experimental? Otherwise, it took me some effort to fetch and generate a diff against Debian experimental to review, and then I'm finding it difficult to understand why you've changed some aspects of the packaging against Debian experimental. For someone else to understand that is key to allowing that difference to be eliminated in the future. What do you think about storing a diff against experimental instead, and then generating that dynamically as part of the script?
[Changes requested] Please could you link directly to the upstream PRs or commits that you are cherry-picking from the patch files themselves? For example there's dep3 which describes what metadata should go into a patch in debian/patches. In this case of squashing multiple commits together, I think that's fine in which case maybe just dump the headers at the top? I don't know of any tool that reads these; it's just really useful when rebasing in the future, so I'd like to make it a standard that the information is available. I think this should be in the source tree itself rather than just in commits.
In order to build upstream mesa snapshot we need to manually prepare source directory. Add support for using the external script instead of just fetching the dsc from Debian. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
In order to evaluate upstream Mesa on the RB1 board, package the snapshot of the Mesa development tree. This package will go away once Mesa 25.2 gets released and enters Debian experimental or unstable. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Import patchset from Mesa MR 35316, implementing 24-bit texture formats support. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
|
@lool @basak-qcom reworked the PR Corresponding packages were uploaded to OBS |
Test jobs for commit 53e17b8 |
There was a problem hiding this comment.
Hey, thanks for implementing this, and sorry for the delay in reviewing on my side; I've left mostly minor comments but please could you:
- change the Debian revision to 0qcom1
- add some notes on how you created the debdiff, either in the commit message or in debian/changelog inside the debdiff
- pass this new script through the shellcheck workflow (.github/workflows/static-checks.yml only runs against ci/*.sh).
Thanks!
| @@ -59,8 +59,24 @@ | |||
| ) | |||
| print("✅ Checksum of original source package matched.") | |||
|
|
|||
| # Unpack the source package | |||
| subprocess.run(['dpkg-source', '-x', dsc_file], cwd=temp_dir, check=True) | |||
| script = config.get('script') | |||
There was a problem hiding this comment.
I guess this could be named "source_download_script"
There was a problem hiding this comment.
Maybe source_unpack_script? Since it replaces the dpkg-source -x call, not the .dsc file download.
There was a problem hiding this comment.
No. It does more than just dpkg-source -x. As such, I'd prefer to keep it as is. I think it should be possible to build an overlay deb without original DSC at all.
| script = os.path.abspath(os.path.join(config_dir, script)) | ||
|
|
||
| env = os.environ.copy() | ||
| env['DSC_FILE'] = dsc_file |
There was a problem hiding this comment.
I'd prefer if we passed the whole yaml file to the script, but I know it's not fun to parse yaml from shell; that's actual actually why I wrote build-deb in python, but now we're extending it to call a shell script :)
I guess this is a temporary solution and we need to get rid of this debdiff juggling logic ASAP, but this will do for now.
| tar xJf ${DEB_FILE} | ||
|
|
||
| cat >> debian/changelog.tmp << EOF | ||
| mesa (${version}-0) experimental; urgency=medium |
There was a problem hiding this comment.
would you mind changing the version to 0qcom1 so that all packages with qcom changes have a qcom version?
|
I like the overall mechanism now - thanks! It's quite clean given the compromise we're already making - still based on a Debian dsc from the Debian snapshot archive, and the script doing the quite minimal upstream snapshot extraction before the debdiff is applied afterwards as normal for the packaging changes. Next I looked at the packaging changes themselves. Here's a summary of all the changes I see to packaging against the base Debian 25.1.0-1 package:
With my Ubuntu background, I'm used to maintaining a delta against an upstream distribution, including upstream snapshots. In that context, there are usually a couple of concerns driven by the desire to converge back with upstream in the future:
For this reason my default expectation is that the delta should be minimal and the reason for each part of the delta existing should be obvious or explained. In Ubuntu we have largely moved to using git to maintain the delta directly, so each part is a commit with an explanation. Here, with everything being squashed into the single debdiff, we don't have that ability yet, unfortunately. Nevertheless I can still apply my usual expectations, consider the debdiff and try to understand why each part is there. How does my default expectation from Ubuntu map to this project? I'm not sure yet! With our "upstream first" philosophy, I think we are agreed that the delta needs to be kept at a minimum. I think the depth we need to spend on the details of the delta itself depends on what user stories we need to support with this package, together with our expectations on how long we will need to maintain this delta for. Is it possible that a mesa snapshot delta isn't short-lived because of another requirement that comes along in the future before the current requirement is upstreamed, for example? Before we used git in Ubuntu, our convention was to explain the delta in I've tweaked the version string to include qcom1 as Loic requested, and also tried to select something that should work for future updates smoothly. Proposed next steps:
With help as above, I'd be happy to take over driving this to getting it landed if you like. |
|
@basak-qcom too many words for a package that is going to be obsoleted in July or August. Could you please make your comments easier to comprehend? |
Update commit ID to a recent one, which, among other changes, pulls in FP16 fixes in the freedreno driver. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Opt-in and use the gallium-rusticl-enable-drivers in order to enable RustiCL drivers which can be enabled by default: asahi, freedreno and radeonsi. The rest of the drivers still need an explicit RUSTICL_ENABLE environment variable. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Test jobs for commit ca86dfe |
Specify debian revision as -0qcom1 instead of just -0 in order to point out that it is a Qualcomm-specific build. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Test jobs for commit ba0c2a3 |
This fixes shellcheck "SC2086 (info): Double quote to prevent globbing and word splitting." Signed-off-by: Robie Basak <robibasa@qti.qualcomm.com>
|
@lumag I pushed 76162c8 directly to your branch to fix some shellcheck warnings. I'm confident that's unobjectionable. I trust that's appropriate process/etiquette here? Three more changes to come: one remaining shellcheck issue, I'll add a README, and I'll rebase onto main in order to pick up the shellcheck CI changes. Please let me know if I should do this differently. |
DSC_FILE is guaranteed to be defined as part of the call API from build-deb.py here. Signed-off-by: Robie Basak <robibasa@qti.qualcomm.com>
Signed-off-by: Robie Basak <robibasa@qti.qualcomm.com>
| # be assigned. Did you mean DEB_FILE? | ||
| # | ||
| # DSC_FILE is guaranteed to be defined as part of the call API from build-deb.py | ||
| # shellcheck disable=SC2153 |
There was a problem hiding this comment.
NAK, don't add extra noise.
| can be enabled by default: asahi, freedreno and radeonsi. | ||
| + Disable gallium-xa and gallium-opencl=icd. | ||
| + No longer need to remove mme_{fermi,tu104}_sim_hw_test | ||
| - debian/control regenerated from debian/control.in. |
There was a problem hiding this comment.
Just add this to the script so that it goes to the actual changelog.
In order to evaluate upstream Mesa on the RB1 board, package the snapshot of the Mesa development tree. This package will go away once Mesa 25.2 gets released and enters Debian experimental or unstable.