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

chore: pull all build scripts under build_files directory #177

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

mulderje
Copy link
Contributor

@mulderje mulderje commented Apr 17, 2024

No intended functionality change here. Move all build*.sh files under a build_files directory. This supports #176. cc: @bsherman

@mulderje mulderje requested a review from castrojo as a code owner April 17, 2024 19:13
@mulderje
Copy link
Contributor Author

@bsherman
Copy link
Contributor

What's the goal here? Seems like moving files should have some organizational purpose but this is a full relocate of all build files. I'd be more interested in moving files somewhat like this, but maybe into distinct dirs, later, when we are splitting into the 3 distinct build flows (see: #176 )

@mulderje
Copy link
Contributor Author

Apologies for the delayed response - I missed your comment somehow...

The goal here is pretty much exactly what you mentioned above; distinct build files per each stream. I think I just took your small changes a little bit too literally, and had put this out while #163 got reviewed to get things started.

With this most recent round of updates, this makes a build_files directory with the below structure, and makes the necessary changes to the build scripts to take the moves into account. One positive action with this change is that it already caught an issue where I had put framework-laptop in both common and extra.

build_files/
├── common
│   ├── build-kmod-evdi.sh
│   ├── build-kmod-openrazer.sh
│   ├── build-kmod-v4l2loopback.sh
│   ├── build-kmod-wl.sh
│   ├── build-kmod-xone.sh
│   ├── build-kmod-xpadneo.sh
│   ├── build-ublue-os-akmods-addons.sh
│   └── ublue-os-akmods-addons
│       └── ublue-os-akmods-addons.spec
├── extra
│   ├── build-kmod-ayaneo-platform.sh
│   ├── build-kmod-ayn-platform.sh
│   ├── build-kmod-bmi260.sh
│   ├── build-kmod-facetimehd.sh
│   ├── build-kmod-framework-laptop.sh
│   ├── build-kmod-gcadapter_oc.sh
│   ├── build-kmod-kvmfr.sh
│   ├── build-kmod-nct6687d.sh
│   ├── build-kmod-openrgb.sh
│   ├── build-kmod-rtl8814au.sh
│   ├── build-kmod-rtl88xxau.sh
│   ├── build-kmod-ryzen-smu.sh
│   ├── build-kmod-vhba.sh
│   ├── build-kmod-VirtualBox.sh
│   └── build-kmod-zenergy.sh
├── nvidia
│   ├── build-kmod-nvidia.sh
│   ├── build-ublue-os-nvidia-addons.sh
│   └── ublue-os-nvidia-addons
│       └── ublue-os-nvidia-addons.spec
└── shared
    └── build-post.sh
    └── build-prep.sh

Eventually I would like to do something like a packages.json to do a bit more validation, and possibly do things like generate the README file. I am thinking something along the lines of the below, but would love any feedback you have there.

[
  {
    "name": "evdi",
    "stream": "common",
    "homepage": "https://www.displaylink.com",
    "description": "kernel module required for use of displaylink",
    "package_source": "negativo17",
    "package_source_details": "fedora-multimedia",
    "package_url": "https://negativo17.org/"
  },
  {
    "name": "facetimehd",
    "stream": "extra",
    "homepage": "https://github.com/patjak/facetimehd/",
    "description": "kernel module Linux driver for the FacetimeHD (Broadcom 1570) PCIe webcam",
    "package_source": "COPR",
    "package_source_details": "mulderje/facetimehd-kmod",
    "package_url": "https://copr.fedorainfracloud.org/coprs/mulderje/facetimehd-kmod/package/facetimehd-kmod"
  }
]

Then we can do things like jq '.[] | select(.stream == "common") | .name' packages.json and compare to an ls as one example.

Also, FYI - I folded #178 into this PR to consolidate. Please let me know if you have any questions/feedback.

Copy link
Contributor

@bsherman bsherman left a comment

Choose a reason for hiding this comment

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

General comment... I don't think any of these changes are necessary, but I do agree that some improvement to organization can be helpful.

I tend to be averse to refactoring for the sake of refactoring, so I look for ways to minimize change. I'm writing this to provide perspective on my comments. I also confess, we all have our own opinions about what is helpful, so I have probably embarked on refactors which others viewed as pointless. :-) I hope that doesn't make me a hypocrite.

@bsherman
Copy link
Contributor

Eventually I would like to do something like a packages.json to do a bit more validation, and possibly do things like generate the README file. I am thinking something along the lines of the below, but would love any feedback you have there.

[
  {
    "name": "evdi",
    "stream": "common",
    "homepage": "https://www.displaylink.com",
    "description": "kernel module required for use of displaylink",
    "package_source": "negativo17",
    "package_source_details": "fedora-multimedia",
    "package_url": "https://negativo17.org/"
  },
  {
    "name": "facetimehd",
    "stream": "extra",
    "homepage": "https://github.com/patjak/facetimehd/",
    "description": "kernel module Linux driver for the FacetimeHD (Broadcom 1570) PCIe webcam",
    "package_source": "COPR",
    "package_source_details": "mulderje/facetimehd-kmod",
    "package_url": "https://copr.fedorainfracloud.org/coprs/mulderje/facetimehd-kmod/package/facetimehd-kmod"
  }
]

Then we can do things like jq '.[] | select(.stream == "common") | .name' packages.json and compare to an ls as one example.

I'm unsure if I want to do something like this for the current repo as it does make it harder to make quick changes. There's something to be said for simple scripts.

I'd like to keep this idea on the backburner for now, or at least make it a distinct PR to discuss after the current changes are merged.

Copy link
Contributor

@bsherman bsherman left a comment

Choose a reason for hiding this comment

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

We'd have minimal changes to the Containerfiles if the COPY line was simpler:

Using common stream as an example:
Instead of COPY build_files/common build_files/shared /tmp/build/
Use COPY build_files/common build_files/shared /tmp/

Then the diffs in the Containerfiles should only apply to substantive changes, like the part which consolidates lines into build-post.sh.

@mulderje
Copy link
Contributor Author

mulderje commented May 2, 2024

We'd have minimal changes to the Containerfiles if the COPY line was simpler:

Using common stream as an example: Instead of COPY build_files/common build_files/shared /tmp/build/ Use COPY build_files/common build_files/shared /tmp/

Then the diffs in the Containerfiles should only apply to substantive changes, like the part which consolidates lines into build-post.sh.

Thanks for the feedback! I have updated the PR to change the directory prefix pack to /tmp vs /tmp/build. Totally agreed that this makes it easier to review the changes. My main motivations for the change were 1) reduce risk of things getting pulled into random directories used in the script (e.g. using dirs under /tmp), and 2) consistency with the blufin repo (I just liked the structure a bit better). Neither of these are worth the churn in this PR.

This should make the changes a lot easier to review and cause a lot less churn. Please let me know if you have any other feedback!

@mulderje mulderje requested a review from bsherman May 2, 2024 04:55
@mulderje
Copy link
Contributor Author

mulderje commented May 2, 2024

Then we can do things like jq '.[] | select(.stream == "common") | .name' packages.json and compare to an ls as one example.

I'm unsure if I want to do something like this for the current repo as it does make it harder to make quick changes. There's something to be said for simple scripts.

I'd like to keep this idea on the backburner for now, or at least make it a distinct PR to discuss after the current changes are merged.

Totally fair! My main motivator here is removing the repetition across all of those build-kmod scripts. Across a large chunk of the scripts the only piece that changes is the package name. (I'll call that the "easy" case below.) The rest will need custom build scripts. My thought was to have the "easy" path here being just needing to add the package name to e.g. the common section in the packages.json file. In this scenario, the rest would just remain their own build-kmod-x.sh files.

I hope that helps clarify what I mean, but I totally understand the value of leaving these alone for simplicity.

@bsherman - FYI I clicked "re-review" here in GH. Please let me know if any other places I need to mark. Happy to chat on discord if easier too.

@mulderje
Copy link
Contributor Author

Updated to include the kvmfr changes from #195

@bsherman - hoping for your thoughts on this, with the previous updates included.

@bsherman
Copy link
Contributor

@bsherman - hoping for your thoughts on this, with the previous updates included.

Sorry I've been distracted with other demands on my time.

I have looked and appreciate you making the changes I requested.

As I mentioned in discord, let's get #201 merged and then this PR updated. Then I'll approve this one.

bsherman
bsherman previously approved these changes May 22, 2024
@mulderje
Copy link
Contributor Author

mulderje commented Jun 5, 2024

Rebased this against main. Any other feedback from the team?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants