Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New package: linux-surface 5.13.13 #32823

Closed
wants to merge 1 commit into from
Closed

Conversation

RononDex
Copy link

@RononDex RononDex commented Sep 3, 2021

New linux kernel for Microsoft Surface devices.
Taken from upstream https://github.com/linux-surface/linux-surface

template file is a copy from the linux kernel 5.13 template

[ci-skip]

General

Have the results of the proposed changes been tested?

  • I use the packages affected by the proposed changes on a regular basis and confirm this PR works for me (Will be using it activly the next few days on my surface device and can then tick this box if stuff works)
  • I generally don't use the affected packages but briefly tested this PR

@ericonr ericonr added the new-package This PR adds a new package label Sep 3, 2021
Copy link
Member

@ericonr ericonr left a comment

Choose a reason for hiding this comment

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

  • Most of the supported archs here don't make sense.
  • Why does this vendor more patches on top of upstream instead of using the github release?
  • Supporting device kernels is icky already, they definitely won't be versioned. Package should be linux-surface directly.

I'm not keen on yet another device kernel, but we might have to cope.

@RononDex
Copy link
Author

RononDex commented Sep 3, 2021

Would it be possible to have this as a restricted package at least?
I am wanting to use void linux on my surface book too. I have already tried this kernel on arch linux on which it worked perfectly.

Having it directly in the upstream repo would be perfect for installing void on surface devices

EDIT: I can hapilly remove all the unneeded archs.
I am not sure what you mean by "github release" (2nd bullet point). The patches are a copy from the linux-surface repo (I have a shell script that always copies over the most up to date ones)

@CameronNemo
Copy link
Contributor

The patches are a copy from the linux-surface repo (I have a shell script that always copies over the most up to date ones)

Why maintain the patches in the void-packages repo, though? There are a lot of them and nobody really knows what they do or how to rebase them if need be. Why not just download the repo and apply the patches from there?

Also FYI they don't seem to support Linux 5.13, you probably want to use 5.10 (which is an LTS kernel). https://github.com/linux-surface/linux-surface/tree/master/patches#maintained-versions

@RononDex
Copy link
Author

RononDex commented Sep 4, 2021

Why maintain the patches in the void-packages repo, though? There are a lot of them and nobody really knows what they do or how to rebase them if need be. Why not just download the repo and apply the patches from there?

That's a fair argument, could adjust the template to do that

Also FYI they don't seem to support Linux 5.13, you probably want to use 5.10 (which is an LTS kernel). https://github.com/linux-surface/linux-surface/tree/master/patches#maintained-versions

I have been running the 5.13 kernel on my surface book (with arch linux) for quite some time now and it works without issues

@q66
Copy link
Contributor

q66 commented Sep 4, 2021

why haven't the patches been mainlined?

@RononDex
Copy link
Author

RononDex commented Sep 4, 2021

why haven't the patches been mainlined?

To be honest I am not sure. Probably because it contains unofficial drivers for proprietary drivers

@RononDex
Copy link
Author

RononDex commented Sep 6, 2021

Just force pushed some requested changes:

  • Downloading needed patches from source with distfiles and using those
  • Dynamically merge the kernel config with the surface-kernel one
  • Remove unnecessary target architectures
  • Renamed generated kernel files to end with "-surface"

@RononDex
Copy link
Author

RononDex commented Sep 6, 2021

I am testing it on my surface book (gen 1) at the moment. It seems the package is having some issues with generating initramfs

Copy link
Member

@Chocimier Chocimier left a comment

Choose a reason for hiding this comment

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

We should wait for upstreaming rather than accept custom kernels outside arm imho.

# configure the kernel; otherwise use arch defaults and all stuff
# as modules (allmodconfig).
local arch subarch
cd $XBPS_BUILDDIR/linux-$majorVersion/linux-$majorVersion
Copy link
Member

Choose a reason for hiding this comment

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

Set build_wrksrc, and look at manual what is current working directory for functions, most of cd won't be needed.
Fix indentaton.

Copy link
Author

Choose a reason for hiding this comment

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

Done

version=5.13.13
revision=3
wrksrc="linux-${version%.*}"
short_desc="Linux kernel and modules containing patches and drivers for Microsoft Surface series devices ( series)"
Copy link
Member

Choose a reason for hiding this comment

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

what series?

Copy link
Author

Choose a reason for hiding this comment

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

Typo, removed

@@ -0,0 +1,7 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered symlinking files/ rather than copying?

Copy link
Author

Choose a reason for hiding this comment

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

Done

homepage="https://github.com/linux-surface/linux-surface"
distfiles="https://cdn.kernel.org/pub/linux/kernel/v5.x/linux-${version%.*}.tar.xz
https://cdn.kernel.org/pub/linux/kernel/v5.x/patch-${version}.xz
https://github.com/linux-surface/linux-surface/archive/refs/tags/arch-${version}-${revision}.tar.gz>surface-linux.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

Revision must be independent from any external resources, use version=5.13.13.3, arch-${version%.*}-${version##*.}

Copy link
Author

Choose a reason for hiding this comment

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

Done, set revision to 1 and version to 5.13.13
Using a static link to the linux-surface distfile

# Template file for 'linux5.13-surface'
pkgname=linux5.13-surface
version=5.13.13
revision=3
Copy link
Member

Choose a reason for hiding this comment

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

Revision must be independent from any external resources. Use version=5.13.13.3, arch-${version%.*}-${version##*.}, set revision to 1.

Copy link
Author

Choose a reason for hiding this comment

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

Done, set revision to 1 and version to 5.13.13
Using a static link to the linux-surface distfile

@RononDex RononDex force-pushed the master branch 3 times, most recently from 4982aac to 996a30c Compare September 7, 2021 15:54
@RononDex
Copy link
Author

RononDex commented Sep 7, 2021

why haven't the patches been mainlined?

To quote qzed from the matrix server:
image

@RononDex
Copy link
Author

RononDex commented Sep 7, 2021

Just tested the new package / kernel on my surface book and stuff works.

However, for full device support more custom built packages (like a custom libwacom, iptsd = ipts daemon, etc) are needed.
The question is, how should I proceed? Is there any chance of getting these packages as a restricted package into this repository?

@q66
Copy link
Contributor

q66 commented Sep 7, 2021

i'm opposed to packaging device-specific things that are upstreamable but they for whatever reason don't, so i'd be against packaging patched libwacom (go tell them to upstream it)

device-specific kernels have a precedent by now (i don't like it, but they're there) so i won't complain about that (fix things blocking review though: rename to linux-surface since there's no way we'll be packaging multiple versions, fix your tabs->spaces issues, simplify the template where appropriate, ...)

@RononDex
Copy link
Author

RononDex commented Sep 8, 2021

I renamed the package to "linux-surface" and fixed the tabs vs spaces indentation

@RononDex
Copy link
Author

RononDex commented Sep 8, 2021

This PR would not be ready for merge imo :)
Tested it and works on my surface book

@q66
Copy link
Contributor

q66 commented Sep 8, 2021

is there supposed to be a dotconfig in the filesdir? the PR does not contain one but as it seems to me there should (otherwise none of the logic for .config gets picked up)

also archs= should be a part of the main metadata block, at the proper line (after revision= IIRC, refer to xlint)

@RononDex
Copy link
Author

RononDex commented Sep 10, 2021

is there supposed to be a dotconfig in the filesdir? the PR does not contain one but as it seems to me there should (otherwise none of the logic for .config gets picked up)

No, in do_configure() the specific architecture dotconfig file (in this case x86_64--dotconfig) is copied into the build directory as dotconfig, along with the specific linux-surface config from the git repo. These are then merged together using the kconfig merge script.

also archs= should be a part of the main metadata block, at the proper line (after revision= IIRC, refer to xlint)

I left it in the same position as it was in the original template from linux5.13. However, I moved it to below revision= now.

@q66
Copy link
Contributor

q66 commented Sep 10, 2021

exactly, I don't see any dotconfig in this PR so the merging will never run

@RononDex
Copy link
Author

RononDex commented Sep 10, 2021

exactly, I don't see any dotconfig in this PR so the merging will never run

They are sym-linked to linux5.13/files, which was a request from the reviewer

EDIT: see here #32823 (comment)

@q66
Copy link
Contributor

q66 commented Sep 10, 2021

ah, I haven't noticed it was a symlink

@q66
Copy link
Contributor

q66 commented Sep 10, 2021

that said, I'm not sure if I'm a fan because that means the kernel will have to be updated strictly in sync with main 5.13, as dotconfigs can change even between patch releases (cc @Chocimier)

@RononDex
Copy link
Author

that said, I'm not sure if I'm a fan because that means the kernel will have to be updated strictly in sync with main 5.13, as dotconfigs can change even between patch releases (cc @Chocimier)

The dotconfig can change and that's completly fine. I still merge it with the custom surface kernel config which takes higher priority. The idea is to have the normal void linux kernel (with all its custom config) supplemented with everything needed to run surface devices

@q66
Copy link
Contributor

q66 commented Sep 10, 2021

it's not fine, because merging and having the base dotconfig be compatible with the current version of linux-surface are two unrelated things

@RononDex
Copy link
Author

So you would prefer to not have it symlinked, but instead have its own config for the base kernel config?

@Chocimier
Copy link
Member

If linking doesn't help to keep config recent, then don't.

@RononDex
Copy link
Author

I have now removed the symbolic link to files and made a local copy of the needed files.

Also made the package a restricted package

New linux kernel for Microsoft Surface devices as a restricted package
Taken from upstream https://github.com/linux-surface/linux-surface
@RononDex
Copy link
Author

Can somebody give me some feedback if this would be mergable this way?

@RononDex
Copy link
Author

@Chocimier @q66 could you please review this PR or tell me if this does not have a chance of making it into the repo?
Thanks

@q66
Copy link
Contributor

q66 commented Sep 25, 2021

I don't have any more comments, but also I don't use x86_64 so I'll leave it to some other maintainer

@Chocimier
Copy link
Member

I still prefer this work being mainlined rather than packaged separately.

@q66
Copy link
Contributor

q66 commented Sep 27, 2021

i think everyone does, but that doesn't help users

@github-actions
Copy link

github-actions bot commented Jun 5, 2022

Pull Requests become stale 90 days after last activity and are closed 14 days after that. If this pull request is still relevant bump it or assign it.

@Anachron
Copy link
Contributor

I was looking for a compatible tablet device that Void supports so I was sad to see this is not merged.

Anybody here using Void on linux surface anyway?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-package This PR adds a new package Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants