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

Redis 7.0.11 #10

Closed
wants to merge 7 commits into from
Closed

Redis 7.0.11 #10

wants to merge 7 commits into from

Conversation

John-Ted
Copy link
Contributor

@John-Ted John-Ted commented Jun 5, 2023

Update redis to 7.0.11

This library was built using musl, some patches are not compatible with pthread-embedded.

Based on #4

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Thank you @John-Ted.

Some other things besides the comments:

  • Update the commit messages and add short descriptions when needed (e.g. explain what new source files were added, explain what patcher were added, explain why histogram was added as a dependency).
  • Sign-off the commits with the same email you added in the Makefile.uk authors.
  • Check that everything that is moved from LIBREDIS_SERVER to LIBREDIS_COMMON makes sense to be moved there. LIBREDIS_COMMON should be a list of everything that is needed both by the server and the client, and it should be minimal.

patches/0004-src-Change-header-path-to-relative.patch Outdated Show resolved Hide resolved
patches/0010-remove-unnecessary-header.patch Outdated Show resolved Hide resolved
Makefile.uk Outdated Show resolved Hide resolved
Makefile.uk Outdated Show resolved Hide resolved
Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

At least on my side, there seem to be a problem with timestamps.

For aarch64, the timestamp appear to be -748823 Jan 1970 -19:-36:00.145, whereas for qemux86_64 it appears to be 28 Jul 2015 07:49:05.074

@John-Ted do you have any idea why this may be happening?

Otherwise, I have been able to test and the things seems fine. 💯

patches/0010-remove-unnecessary-header.patch Outdated Show resolved Hide resolved
@John-Ted
Copy link
Contributor Author

In regards to the timestamps, on ARM the gettimeofday call does not function properly. On x86, I tracked it down to this 1, which calls their version of localtime with the server.timezone parameter, which is subtracted from the timestamp 2. This variable has the value 263675520, which corresponds to a shift of about 8 years, consistent with the change from 2023 to 2015.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Works great @John-Ted, thank you.
Please start your commit messages with a capital letter.
Also see the changes to the commits below:

  • bump version to 7.0.11 -> Bump version from ---- to 7.0.11
    Add a description of what changes needed to be made, new dependencies, new patches, etc.
  • refactor code into single library -> Makefile.uk: Refactor code into single library
    Also add a description here, state that previously there were separate libraries for
    LIBREDIS_SERVER, LIBREDIS_CLIENT, LIBREDIS_COMMON, now we do the separation
    by config options.
  • improve patches -> patches: Impove patches
    Also, I would only leave this to remove the unnecessary patches, and update patch 6 in
    the commit that adds it.
    So in the end the commit would be patches: Remove unnecesarry patches.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

@John-Ted should this commit be part of this one? We should try to keep it as bisect-friendly as possible, ideally if you check out on any of the commits the library should compile fine.

Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

For the commit 1b46aa5 ( patches: Remove unnecessary patches) , please state why these patches are not necessarry anymore.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Looks fine to me besides the one comment @John-Ted, I'll wait for @RaduNichita to also test on his side.

LIBREDIS_COMMON_SRCS-y += $(LIBREDIS_SRC)/release.c
LIBREDIS_COMMON_SRCS-y += $(LIBREDIS_SRC)/siphash.c
LIBREDIS_COMMON_SRCS-y += $(LIBREDIS_SRC)/zmalloc.c
LIBREDIS_SRCS-y += $(LIBREDIS_SRC)/adlist.c
Copy link
Member

Choose a reason for hiding this comment

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

Please order the LIBREDIS_SRCS source files in lexicographic order.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good on my side, thank you. I'll add the tag after @RaduNichita is happy.

Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

All good with me now. I am happy!

Reviewed-by: Radu Nichita radunichita99@gmail.com

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Approved-by: Stefan Jumarea stefanjumarea02@gmail.com

Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Histogram is a new dependency introduced by Redis

Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Add new source files introduced by Redis

Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Because we use musl, several patches that changed pthreads calls
to work with newlib are no longer needed and have been removed.
Removed a patch that would remove an `#include` directive from
the code as that header is now necessary. Removed a patch that
would add a relative import path to a header, as that path is
part of `CINCLUDES`. Added a patch that removes a header which
causes compilation errors.

Signed-off-by: Ioan-Teodor Teugea <ioan_teodor.teugea@stud.acs.upb.ro>
Previously, the code was compiled into 3 different libraries,
LIBREDIS_SERVER, LIBREDIS_CLIENT and LIBREDIS_COMMON. This commit
changes this so the code is compiled into a single library,
LIBREDIS, which enables the server or the client functionality
based on the config options.

Signed-off-by: Ioan-Teodor Teugea <ioan_teodor.teugea@stud.acs.upb.ro>
unikraft-bot pushed a commit that referenced this pull request Aug 4, 2023
Histogram is a new dependency introduced by Redis

Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #10
unikraft-bot pushed a commit that referenced this pull request Aug 4, 2023
Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #10
unikraft-bot pushed a commit that referenced this pull request Aug 4, 2023
Add new source files introduced by Redis

Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #10
unikraft-bot pushed a commit that referenced this pull request Aug 4, 2023
Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #10
unikraft-bot pushed a commit that referenced this pull request Aug 4, 2023
Because we use musl, several patches that changed pthreads calls
to work with newlib are no longer needed and have been removed.
Removed a patch that would remove an `#include` directive from
the code as that header is now necessary. Removed a patch that
would add a relative import path to a header, as that path is
part of `CINCLUDES`. Added a patch that removes a header which
causes compilation errors.

Signed-off-by: Ioan-Teodor Teugea <ioan_teodor.teugea@stud.acs.upb.ro>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #10
unikraft-bot pushed a commit that referenced this pull request Aug 4, 2023
Previously, the code was compiled into 3 different libraries,
LIBREDIS_SERVER, LIBREDIS_CLIENT and LIBREDIS_COMMON. This commit
changes this so the code is compiled into a single library,
LIBREDIS, which enables the server or the client functionality
based on the config options.

Signed-off-by: Ioan-Teodor Teugea <ioan_teodor.teugea@stud.acs.upb.ro>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/merged enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants