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 |
basak-qcom
left a comment
basak-qcom
left a comment
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.
|
@lool @basak-qcom reworked the PR Corresponding packages were uploaded to OBS |
Test jobs for commit 53e17b8 |
lool
left a comment
lool
left a comment
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!
|
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? |
Test jobs for commit ca86dfe |
Test jobs for commit ba0c2a3 |
|
@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. |
basak-qcom
left a comment
basak-qcom
left a comment
There was a problem hiding this comment.
I think we need to talk about CI and lint tools.
The culture I think we need to be heading towards is:
- Use linting in CI to give immediate contributor feedback and achieve a baseline of quality, both for our repositories and to set an example across the company.
- Lint tools are indeed dumb. They will make us do things that aren't necessary. That's the cost of achieving the previous point, but the benefit is worth the cost.
- When the lint tool is wrong, a simple override should be enough, using whatever mechanism the lint tooling provides. Best practice is to explain how the lint tool is wrong in a comment, to avoid suppressing the lint tooling when not appropriate ensuring that this can be reviewed effectively.
- If the linter is wrong, just override it and move on. I don't think it's worth the effort of trying to figure out how to work around the linter so that it won't alert.
- Fixing the linter is out of scope. Perfect is the enemy of the good. We have better uses for our time. Just using the linter gives us enough bang for the buck.
- This does mean apparently unnecessary noise, but the general best practice I'm used to is that it's worth the cost.
I'm surprised to see this being nitpicked here, and indeed this isn't something that's worth debating in a PR. The above should just be in a documented best practice document so that we do not endlessly debate this in PRs.
|
@basak-qcom No, no and no. If linters require changing the source code, then those linters should go to /dev/null. I don't want to worry about adding extra comments to make the tool happy. The tools must be helping us, not the other way around. If's fine to provide a config file for the tool or a set of args in the makefile (like we do for -Wsomething for cc), but having to add comments to C source file doesn't make sense. |
|
The linter was already there. I just fixed it to scan the script you're adding here, rather than just If you want to change how we do linting as a project, then presumably that's a general policy question and an issue for a different PR. If you refuse to add a lint override to the script you're adding here, then this PR is blocked on that. |
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 <[email protected]>
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 <[email protected]>
Import patchset from Mesa MR 35316, implementing 24-bit texture formats support. Signed-off-by: Dmitry Baryshkov <[email protected]>
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 <[email protected]>
DSC_FILE is guaranteed to be defined as part of the call API from build-deb.py here. Signed-off-by: Robie Basak <[email protected]> Signed-off-by: Dmitry Baryshkov <[email protected]>
Ignore the "Double quote to prevent globbing and word splitting" error from shellcheck. Script authors should have knowledge whether the variable can contain spaces or shell glob symbols or not. Signed-off-by: Dmitry Baryshkov <[email protected]>
|
I went through all the requested changes and saw that the latest pull request was addressing all, so closed them and will merge; thanks! |
|
(NB: build failure is unrelated to this PR / repo and is being fixed in a separate PR.) |
|
@lumag I kicked a build, but it failed due to git not being installed: I'd suggest either adding a sudo apt install git to the shell script, or to add an entry in the yaml with list of dependencies to install prior to running the script. |
4 participants
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.