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

Add dkp-toolchain-vars package to docker images #28

Closed
wants to merge 1 commit into from

Conversation

TSRBerry
Copy link

This PR adds the dkp-toolchain-vars package to the docker images.

Even if this might not be necessary this helps for CI scenarios where packages that define "dkp-toolchain-vars" as a make dependency in their PKGBUILD files currently can't be build. Since we can't install packages during CI execution we need to include it in the image (as using the ignore dependencies option is even worse).

@WinterMute
Copy link
Member

It isn't appropriate to build and install packages as part of CI, this is a waste of resources. Please PR packages to https://github.com/devkitPro/pacman-packages rather than continuing to waste time and energy building and installing dependencies for every commit.

@WinterMute WinterMute closed this Nov 7, 2023
@TSRBerry
Copy link
Author

TSRBerry commented Nov 7, 2023

Well I would love to stop doing that, but I don't know how. Do you have any ideas/hints for me?
I'm not sure why you are pointing me to the packages repo, because this package is already there and I just need it, so CI is able to build the PKGBUILD files I provide.

@WinterMute
Copy link
Member

WinterMute commented Nov 7, 2023

Again. Building and installing packages in CI is not appropriate. It wastes time and energy. The appropriate way to provide dependencies is to PR the appropriate PKGBUILD file to https://github.com/devkitPro/pacman-packages from where it will be deployed as part of the docker images we provide in order to facilitate CI.

Do not build and install packages as part of CI

Bypassing blocks on using dkp-pacman as part of CI causes problems for us by encouraging others to make a mess of their installations in order to build projects which contain massively inappropriate and unnecessary build commands which ultimately propagate technical debt across projects.

@TSRBerry
Copy link
Author

Adding the PKGBUILD files for these two libraries comes with other issues, doesn't it?

If @DarkMatterCore wants to change anything in these files he needs to PR the changes to you before they can be integrated into https://github.com/DarkMatterCore/nxdumptool and I'm not sure if that's ideal.

Ideally I'd like to get rid of this line in the following workflow, because I don't think we should need to rely on a third party (even though leseratte is trustworthy) to install missing dkp dependencies:
https://github.com/DarkMatterCore/nxdumptool/blob/98ed7d1c29616174a11fb7791042cb3eb3e40642/.github/workflows/rewrite.yml#L31-L32

I understand if you don't want to do it by providing this package in the docker image, but I don't know how else to get rid of that line in the workflow. If you don't mind could I ask what you think would be the best way to accomplish that?

@WinterMute
Copy link
Member

Adding the PKGBUILD files for these two libraries comes with other issues, doesn't it?

No.

(even though leseratte is trustworthy)

Leserrate is not trustworthy. He's been asked over & over again to stop rehosting our binaries and encouraging people to use mismatched packages to build projects instead of updating them to build properly without bodging our tools around. He's been blocked from our repos and banned from our servers yet he insists on bypassing blocks and banlists in order to rehost our binaries so that people can maintain a pile of technical debt without cleaning it up.

@DarkMatterCore
Copy link

@WinterMute Are you willing to review individual PRs to the pacman-packages repository to add both filesystem libraries if I create them myself?

I'll be honest, I didn't even bother to try after devkitPro/pacman-packages#113 got stuck in PR hell. Scires himself has stated multiple times (mostly on Discord) that he's not interested in implementing a UMS driver into Atmosphère, and he's perfectly fine with people using a statically linked driver into each homebrew application where the feature is needed/desired.

@WinterMute
Copy link
Member

@WinterMute Are you willing to review individual PRs to the pacman-packages repository to add both filesystem libraries if I create them myself?

Yes, although the liblwext4 patch seems way over the top. Why have all the headers been moved?

I'll be honest, I didn't even bother to try after devkitPro/pacman-packages#113 got stuck in PR hell.

I'm not sure I'd characterise a PR sitting unmerged as "PR hell". As far as we were aware nobody was using this & we do consider the best way to handle extra filesystems as some kind of sysmodule/service accessible to all homebrew rather than linked into every homebrew app that wants to use them.

@DarkMatterCore
Copy link

Yes, although the liblwext4 patch seems way over the top. Why have all the headers been moved?

Having most of the headers files directly under $(PORTLIBS)/switch/include didn't sit right to me. Maybe it'd be okay if it was one or two header files referencing other header files inside a subdirectory instead of having the whole bunch of them sitting right there, but it's not the case.

I admit the patch is a bit invasive on that regard, but I only did it to warrant better file organization. Other portlibs follow a similar layout to avoid cluttering the include directory, so I just wanted to replicate that behavior.

If it's not acceptable, just let me know. I can revert that change on my project's dev branch.

As far as we were aware nobody was using this & we do consider the best way to handle extra filesystems as some kind of sysmodule/service accessible to all homebrew rather than linked into every homebrew app that wants to use them.

Back in Q4 2019, I was working alongside XorTroll on fsp-usb, a custom sysmodule for UMS device management that would have eventually made its way into Atmosphère if it wasn't for all the issues we encountered down the road.

The amount of memory available for system processes is very, very limited. It isn't unusual at all for users to run into issues if they're using three or more custom sysmodules. It isn't unusual for Atmosphère itself to run into memory problems of its own as well -- just look at past issues related to RomFS table building, which have forced the project to steal memory from other processes.

In the end, this severely affected what we could and couldn't do. Supporting more than one drive at a time, improving transfer speeds or supporting more than just FAT filesystems was proving to be difficult, which is why we decided to switch to a static userland library/driver after discussing the topic with Scires (who seemed to agree at the time).

Hopefully that helps to shed some light on this particular matter.

@WinterMute
Copy link
Member

This is getting slightly out of scope for this PR :P

Would you mind continuing at https://devkitpro.org/viewforum.php?f=42

@DarkMatterCore
Copy link

This is getting slightly out of scope for this PR :P

Would you mind continuing at https://devkitpro.org/viewforum.php?f=42

Sure, no problem.

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.

None yet

3 participants