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

upgrade vcgencmd without tmp vulnerability and inclusion for bananapi… #5100

Closed
wants to merge 1 commit into from

Conversation

PanderMusubi
Copy link
Contributor

Description

Upgrade vcgencmd without tmp vulnerability and inclusion for bananapim2ultra

See also #5095 (comment)

How Has This Been Tested?

  • Pine64
  • Rock64
  • Banana Pi M2 Ultra

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Copy link
Member

@rpardini rpardini left a comment

Choose a reason for hiding this comment

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

You're copy/pasting code and duplicating it. Extract the code into a generic extension.
btw how is this useful at all? why pollute pure mainline boards with vcgencmd stuff?

@PanderMusubi
Copy link
Contributor Author

Adding vcgencmd allows monitoring and managing (reboot etc) Armbian from the all Raspi Check Android app which is very handy. It is only relevant for the non-Raspberry Pi builds. So all build but one. Discussed this in Discord. If you can guide me his to do that in another way, led me know. When adding this to the RPI4b build via a generic solution, it clashes with vcgencmd installed via a standard RPi package.

@EvilOlaf
Copy link
Member

You're copy/pasting code and duplicating it. Extract the code into a generic extension. btw how is this useful at all? why pollute pure mainline boards with vcgencmd stuff?

Agreed. A tutorial on how to install it should be sufficient for those who need it.

@PanderMusubi
Copy link
Contributor Author

The Android app that uses it is also aimed at users with leas experience. Would packaging it as a deb make it easier to get supported?

@rpardini
Copy link
Member

While I appreciate the effort, adding junk just to support one random Android app is a bit out of scope.
My suggestion: investigate the extensions mechanism, write an extension, you can even PR it. This will be optional, but you can use a config file in your userpatches to enable it. You can make the extension "do nothing" if enabled against a bcm2711 family so it's universal.
That way, we all benefit: the extension is available to all who wanna build that way, but doesn't bother users who don't want an unknown, possibly vuln-riddled thing in their builds.

@rpardini rpardini closed this Apr 27, 2023
@PanderMusubi
Copy link
Contributor Author

Thanks. Seems like indeed a better way.

@rpardini
Copy link
Member

Ok, but please also send a revert for #5095

@PanderMusubi
Copy link
Contributor Author

While I appreciate the effort, adding junk just to support one random Android app is a bit out of scope. My suggestion: investigate the extensions mechanism, write an extension, you can even PR it. This will be optional, but you can use a config file in your userpatches to enable it. You can make the extension "do nothing" if enabled against a bcm2711 family so it's universal. That way, we all benefit: the extension is available to all who wanna build that way, but doesn't bother users who don't want an unknown, possibly vuln-riddled thing in their builds.

Small question, implementing it as an extension, the user has to build with it explicitly it themselves? Because the target audience for offering this implementation already installed is mainly inexperienced users that don't build it themselves.

Would it be better perhaps to just add a paragraph to the documentation as one wget command will install it?

@rpardini
Copy link
Member

Yep. if you create a extensions/vcgencmd_fake.sh, then you can enable it with adding EXT=vcgencmd_fake to the build command line.

@PanderMusubi PanderMusubi mentioned this pull request May 3, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants