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.

Sign up for GitHub

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

Jump to bottom

feat: make search path for BinaryFinder customizable. #43968

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: make search path for BinaryFinder customizable. #43968

wants to merge 2 commits into from
+132 −10

Conversation

exi
Copy link

@exi exi commented Mar 3, 2024 •
edited
Loading

Summary

This feature is important for nextcloud running on distributions like NixOS, where all the standard
search paths do not exist.

I chose to make the list overridable instead of appendable because having a default list that always gets
applied might cause reproducibility issues on distros like NixOS where we have multiple versions
of the same binary at the same time.

This fixes issue #43922

Checklist

@exi exi force-pushed the custom-binary-search-paths branch 2 times, most recently from 68c348b to 2d5a7fb Compare March 3, 2024 21:53
@exi exi marked this pull request as ready for review March 3, 2024 22:00
@exi exi mentioned this pull request Mar 4, 2024
Open
@exi exi force-pushed the custom-binary-search-paths branch 2 times, most recently from e18dbbf to a00f68b Compare March 4, 2024 13:47
@exi
Copy link
Author

exi commented Mar 5, 2024

@kesselb i force pushed a change to address the workflow failures. It seems to need another review/approval now

@joshtrichards joshtrichards added enhancement 3. to review Waiting for reviews labels Mar 5, 2024
@exi exi force-pushed the custom-binary-search-paths branch 8 times, most recently from dee2942 to 30d13cd Compare March 6, 2024 13:30
@exi
Copy link
Author

exi commented Mar 6, 2024

@kesselb every time, all the tests checking out submodules fail for branches that don't originate from github.com/nextcloud. Is this something I'm doing wrong, or are the CI checks just funky?

I don't know what I did to cause the errors in the failing actions.

@skjnldsv
Copy link
Member

skjnldsv commented Mar 7, 2024

@exi the cypress will fail, it's broken for forks.

@skjnldsv skjnldsv requested review from a team, ArtificialOwl, icewind1991 and Fenn-CS and removed request for a team March 7, 2024 21:31
@skjnldsv skjnldsv added this to the Nextcloud 30 milestone Mar 7, 2024
@exi exi force-pushed the custom-binary-search-paths branch 2 times, most recently from 956e961 to 7c8769b Compare March 8, 2024 08:58
kesselb
kesselb reviewed Mar 8, 2024
View reviewed changes
tests/lib/BinaryFinderTest.php
protected function setUp(): void {
$this->oldEnv = getenv("PATH");
// BinaryFinder always includes the "PATH" environment variable into the search path,
// which we want to avoid in this test because they are not usually found in webserver
Copy link
Contributor

@kesselb kesselb Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!

Why should the path environment variable not be available in webserver deployments?

Copy link
Author

Choose a reason for hiding this comment

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

Apache does not set or pass it through by default. Phpfpm does not set it as well. Not sure about others but I would suspect that most servers don't provide it by default.

@exi
Copy link
Author

exi commented Mar 12, 2024

What is blocking this from being merged? As far as I can tell, all necessary tests pass except for the ones that always fail.

@kesselb
Copy link
Contributor

kesselb commented Mar 13, 2024

What is blocking this from being merged?

master is still Nextcloud 29.
We can merge it after the branch off.

@exi
Copy link
Author

exi commented May 3, 2024

@kesselb 29.0.0 has been tagged, is this ready for merge now?

@kesselb
Copy link
Contributor

kesselb commented May 3, 2024

Yup, another rebase would be nice.

@exi exi force-pushed the custom-binary-search-paths branch from 3d307d5 to fa21a42 Compare May 3, 2024 20:10
@exi
Copy link
Author

exi commented May 3, 2024

rebased it

@kesselb kesselb added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 3, 2024
@kesselb kesselb force-pushed the custom-binary-search-paths branch from fa21a42 to 45518de Compare May 5, 2024 15:04
@exi @icewind1991
feat: make search path for BinaryFinder customizable.
8f88459 
This feature is important for nextcloud running on
distributions like NixOS, where all the standard
search paths do not exist.

Also added tests.

This fixes issue nextcloud#43922

Signed-off-by: Reno Reckling <e-github@wthack.de>
@joshtrichards
Merge branch 'master' into custom-binary-search-paths
b6ee11e 
Signed-off-by: Josh <josh.t.richards@gmail.com>
kesselb
kesselb requested changes May 27, 2024
View reviewed changes
Copy link
Contributor

@kesselb kesselb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Blocking for now.
Needs approval by @nickvergessen.

nickvergessen
nickvergessen reviewed Jun 6, 2024
View reviewed changes
lib/private/BinaryFinder.php
public function __construct(
ICacheFactory $cacheFactory,
private IConfig $config
) {
$this->cache = $cacheFactory->createLocal('findBinaryPath');
Copy link
Member

Choose a reason for hiding this comment

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

Can we assume people are clever enough to purge the local cache when changing the config?
Or should we add e.g. md5(json_encode($this->config->getSystemValue('binary_search_paths'))) to the cache prefix?

Copy link
Member

Choose a reason for hiding this comment

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

The cache is only for 5 minutes + we currently cache not found/fallback results and find that acceptable. So I don't think it's necessary.

joshtrichards
joshtrichards approved these changes Jul 13, 2024
View reviewed changes
Copy link
Member

@joshtrichards joshtrichards left a comment

Choose a reason for hiding this comment

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

Let's get this in.

lib/private/BinaryFinder.php
public function __construct(
ICacheFactory $cacheFactory,
private IConfig $config
) {
$this->cache = $cacheFactory->createLocal('findBinaryPath');
Copy link
Member

Choose a reason for hiding this comment

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

The cache is only for 5 minutes + we currently cache not found/fallback results and find that acceptable. So I don't think it's necessary.

lib/private/BinaryFinder.php
Comment on lines +55 to 59
null,
$this->config->getSystemValue('binary_search_paths', self::DEFAULT_BINARY_SEARCH_PATHS));
if ($result === null) {
$result = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
null,
$this->config->getSystemValue('binary_search_paths', self::DEFAULT_BINARY_SEARCH_PATHS));
if ($result === null) {
$result = false;
}
false,
$this->config->getSystemValue('binary_search_paths', self::DEFAULT_BINARY_SEARCH_PATHS));

An opportunity to streamline. :-)

This was referenced Jul 30, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@kesselb kesselb kesselb requested changes

@icewind1991 icewind1991 icewind1991 approved these changes

@joshtrichards joshtrichards joshtrichards approved these changes

@Fenn-CS Fenn-CS Fenn-CS approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@Altahrim Altahrim Awaiting requested review from Altahrim

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

Configurable binary path for Mailer/etc
7 participants
@exi @skjnldsv @kesselb @nickvergessen @icewind1991 @joshtrichards @Fenn-CS