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

fix: make Dragonfly compatible with older systems #1755

Merged
merged 1 commit into from
Aug 29, 2023
Merged

fix: make Dragonfly compatible with older systems #1755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,3 @@ include_directories(helio)

add_subdirectory(helio)
add_subdirectory(src)

include(cmake/Packing.cmake)
7 changes: 6 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,16 @@ HELIO_OPENSSL_USE_STATIC_LIBS = ON
HELIO_ENABLE_GIT_VERSION = ON
HELIO_WITH_UNWIND = OFF

# equivalent to: if $(uname_m) == x86_64 || $(uname_m) == amd64
ifneq (, $(filter $(BUILD_ARCH),x86_64 amd64))
Copy link
Contributor

Choose a reason for hiding this comment

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

Also isn't this only for the architecture and for the processor family ? I would expect something here that looks the microprocessor architecture and its familly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, we support x86_64 and arm64 architectures and we release binaries that can run for both architectures.
See here: https://github.com/dragonflydb/dragonfly/releases

Copy link
Contributor

Choose a reason for hiding this comment

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

I know which architectures we support, it appeared to me that this PR was meant to address the microarchitecture mentioned on that PR. I got confused, that's all

HELIO_MARCH_OPT := -march=core2 -msse4.1 -mpopcnt -mtune=skylake
Copy link
Contributor

Choose a reason for hiding this comment

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

I think skylake is a mistake here. They are running cpu family 6 model 13 which according to this is a pentium m. So you are tuning for an architecture that came later. Furthermore, as per the comments, shouldn't it be -march=native?

What am I missing here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Our responsibility as project maintainers to do what's right and not what was asked in the comments :)

We lowered the bar but we are not going to support 32bit processor pentium-m discontinued in 2009.
march=native will produce a code based on Azure github runner CPU. That's totally random and will be incompatible with many machines that run Dragonfly today. I will update the commit description

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 I got confused because you wrote Possibly addresses and the changes introduced in this PR should be incompatible with what is discussed on that thread.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, fixed the description.

endif

HELIO_FLAGS = $(if $(HELIO_RELEASE),-release $(HELIO_RELEASE_FLAGS),) \
-DBoost_USE_STATIC_LIBS=$(HELIO_USE_STATIC_LIBS) \
-DOPENSSL_USE_STATIC_LIBS=$(HELIO_OPENSSL_USE_STATIC_LIBS) \
-DENABLE_GIT_VERSION=$(HELIO_ENABLE_GIT_VERSION) \
-DWITH_UNWIND=$(HELIO_WITH_UNWIND) \
-DWITH_UNWIND=$(HELIO_WITH_UNWIND) -DMARCH_OPT="$(HELIO_MARCH_OPT)"

.PHONY: default

Expand Down