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

Fix 32bit support and add workflow for 32bits testing #36120

Merged
merged 32 commits into from
Feb 7, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Jan 12, 2023

Signed-off-by: Côme Chilliet [email protected]

Summary

Add 32bits to CI and fix errors

TODO

  • Have a working 32bits PHPUnit CI step
  • Refine which tests it runs or not
  • Fix errors
  • Refine when this CI step runs or not
  • Backport CI step (and fixes if needed) to stable25

Checklist

@come-nc come-nc added the 2. developing Work in progress label Jan 12, 2023
@come-nc come-nc self-assigned this Jan 12, 2023
@come-nc come-nc added this to the Nextcloud 26 milestone Jan 12, 2023
@come-nc
Copy link
Contributor Author

come-nc commented Jan 12, 2023

There were 2 failures:

1) Test\User\UserTest::testSetQuota
Expectation failed for method name is "setUserValue" when invoked 1 time(s)
Parameter 3 for invocation OCP\IConfig::setUserValue('foo', 'files', 'quota', '0 B', null) does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'23 TB'
+'0 B'

/__w/server/server/lib/private/User/User.php:526
/__w/server/server/tests/lib/User/UserTest.php:744

2) Test\User\UserTest::testSetQuotaAddressNoChange
OCP\IConfig::setUserValue('foo', 'files', 'quota', '0 B', null) was not expected to be called.

/__w/server/server/lib/private/User/User.php:526
/__w/server/server/tests/lib/User/UserTest.php:831

So it works.

But either we do not have enough tests on quota or they are not run by the selection I’ve put, because I expected type errors as well on file sizes.
We need to add tests specifically for files over 4GB I guess.

There is also a whole bunch of:

1) OCA\DAV\Tests\unit\CalDAV\CalDavBackendTest::testCalendarQuery with data set "all" (array(0, 1, 2, 3), array(), array())
ValueError: Epoch doesn't fit in a PHP integer

/__w/server/server/apps/dav/lib/CalDAV/CalDavBackend.php:2738
/__w/server/server/apps/dav/lib/CalDAV/CalDavBackend.php:1209
/__w/server/server/apps/dav/tests/unit/CalDAV/AbstractCalDavBackend.php:221
/__w/server/server/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php:426

@come-nc

This comment was marked as outdated.

@come-nc come-nc force-pushed the enh/32bits-support branch from eb07eae to 36990e3 Compare January 20, 2023 16:28
@come-nc come-nc force-pushed the enh/32bits-support branch from 5cc4d14 to 045a249 Compare January 23, 2023 10:41
@come-nc
Copy link
Contributor Author

come-nc commented Jan 23, 2023

For the ValueError: Epoch doesn't fit in a PHP integer errors from dav tests, they are triggered by calling getTimestamp on DateTime objects for dates after 2038.
We need to decide whether we want to support events after 2038 in dav/calendar or not for 32bits, which would imply retyping all timestamps in there to int|float. Other solution is to document it as known limitation on 32bits and skip those tests on 32bits.
ping @PVince81 @AndyScherzinger

@AndyScherzinger
Copy link
Member

support events after 2038

I'd be fine to document it as a known limitation, that 32-bit can't handle dates past that time.

@come-nc
Copy link
Contributor Author

come-nc commented Jan 23, 2023

  • Document that events past 2038 (check actual limit) are not supported on 32bit

@come-nc come-nc added the pending documentation This pull request needs an associated documentation update label Jan 23, 2023
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews 2. developing Work in progress and removed 2. developing Work in progress 3. to review Waiting for reviews labels Jan 24, 2023
@ChristophWurst ChristophWurst marked this pull request as draft January 24, 2023 07:06
@ChristophWurst
Copy link
Member

review was wrongly requested because the PR was set for review already. put it back to draft.

Also fixed numericToString to correctly convert float to int if it fits

Signed-off-by: Côme Chilliet <[email protected]>
Because the parameter type was moved to phpdoc it needs to be removed
 from implementations

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc force-pushed the enh/32bits-support branch from 4204565 to 6e276ad Compare February 7, 2023 10:23
@come-nc come-nc merged commit 4e969ef into master Feb 7, 2023
@come-nc come-nc deleted the enh/32bits-support branch February 7, 2023 13:08
@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 7, 2023
@blizzz blizzz mentioned this pull request Feb 9, 2023
@DaphneMuller
Copy link

hello @come-nc
Thank you so much for your work on this pull request! This ticket seems to have the tag 'pending documentation'.
Is there any chance you could clarify what documentation is missing? Is this for admins or for app developers?

@come-nc come-nc removed the pending documentation This pull request needs an associated documentation update label Feb 21, 2023
@MichaIng
Copy link
Member

@come-nc
Why was this never backported to NC25? It is high likely what fixes quotas on NC26 while it is still broken on NC25. It is is overall to much/large, we could also cherry-pick some commits?

@come-nc
Copy link
Contributor Author

come-nc commented Mar 27, 2023

@come-nc Why was this never backported to NC25? It is high likely what fixes quotas on NC26 while it is still broken on NC25. It is is overall to much/large, we could also cherry-pick some commits?

Several reasons, mainly this was fixed through strong typing using union type int|float which is only available on PHP 8.0 and higher, and thus cannot be used on 25.
So backporting would have meant doing it again using phpdoc instead of real typing.

Also you can see I had to change things in lib/public (OCP), which we tend to avoid on bugfix releases.

@MichaIng
Copy link
Member

MichaIng commented Mar 27, 2023

Okay, makes sense. Then it will be some detectives work to find the part which fixes quota usage on Nextcloud 25. Or the monkeys task to translate all union types to phpdoc comments for backporting everything 😄. I'll see if I can do that, excluding CI and unrelated stuff.

Also you can see I had to change things in lib/public (OCP), which we tend to avoid on bugfix releases.

In this cases it fixes bugs, so I think it is necessary. But I also think most of these are in NC25 already (phpdoc) via #35734 and some other PRs.

MichaIng added a commit that referenced this pull request Apr 21, 2023
This backports most changes from:
#36120

Excludes union type, supported by PHP 8.x only
replaced with phpdoc in case.

Signed-off-by: MichaIng <[email protected]>
@MichaIng MichaIng added the feature: 32bits Bug specific to 32bits architectures label Apr 21, 2023
@MichaIng
Copy link
Member

PR up which backports most of the changes done here: #37877

MichaIng added a commit that referenced this pull request Apr 21, 2023
This backports most changes from:
#36120

Excludes union type, supported by PHP 8.x only
replaced with phpdoc in case.

Signed-off-by: MichaIng <[email protected]>
MichaIng added a commit that referenced this pull request Apr 21, 2023
This backports most changes from:
#36120

Excludes union type, supported by PHP 8.x only
replaced with phpdoc in case.

Signed-off-by: MichaIng <[email protected]>
MichaIng added a commit that referenced this pull request Apr 24, 2023
This backports most changes from:
#36120

Excludes union type, supported by PHP 8.x only
replaced with phpdoc in case.

Signed-off-by: MichaIng <[email protected]>
MichaIng added a commit that referenced this pull request May 17, 2023
This backports most changes from:
#36120

Excludes union type, supported by PHP 8.x only
replaced with phpdoc in case.

Signed-off-by: MichaIng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: 32bits Bug specific to 32bits architectures
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring back 32 bit support on Nextcloud 26
7 participants