-
Notifications
You must be signed in to change notification settings - Fork 41
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
base: main
Are you sure you want to change the base?
Conversation
Build result preview in mulderje#8 https://github.com/mulderje/akmods/actions/runs/8727451953?pr=8 |
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 ) |
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
Eventually I would like to do something like a [
{
"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 Also, FYI - I folded #178 into this PR to consolidate. Please let me know if you have any questions/feedback. |
There was a problem hiding this 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.
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. |
There was a problem hiding this 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
.
Thanks for the feedback! I have updated the PR to change the directory prefix pack to 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! |
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 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. |
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. |
Rebased this against main. Any other feedback from the team? |
No intended functionality change here. Move all
build*.sh
files under a build_files directory. This supports #176. cc: @bsherman