Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(makefile): Bugfixes and minor improvements #10423

Merged
merged 1 commit into from
Apr 25, 2023
Merged

fix(makefile): Bugfixes and minor improvements #10423

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
30 changes: 20 additions & 10 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,10 @@ QML_DEBUG_PORT ?= 49152

ifneq ($(QML_DEBUG), false)
DOTHERSIDE_CMAKE_PARAMS := -DCMAKE_BUILD_TYPE=Debug -DQML_DEBUG_PORT=$(QML_DEBUG_PORT)
DOTHERSIDE_BUILD_CMD := cmake --build . --config Debug $(HANDLE_OUTPUT)
DOTHERSIDE_BUILD_CMD := cmake --build . --config Debug
else
DOTHERSIDE_CMAKE_PARAMS := -DCMAKE_BUILD_TYPE=Release
DOTHERSIDE_BUILD_CMD := cmake --build . --config Release $(HANDLE_OUTPUT)
DOTHERSIDE_BUILD_CMD := cmake --build . --config Release
endif

MONITORING ?= false
Expand Down Expand Up @@ -265,6 +265,7 @@ $(STATUSQ_CMAKE_CACHE): | deps
$(STATUSQ_CMAKE_CONFIG_PARAMS) \
-B $(STATUSQ_BUILD_PATH) \
-S ui/StatusQ \
-Wno-dev \
$(HANDLE_OUTPUT)

statusq-configure: | $(STATUSQ_CMAKE_CACHE)
Expand All @@ -273,8 +274,7 @@ statusq-build: | statusq-configure
echo -e "\033[92mBuilding:\033[39m StatusQ"
cmake --build $(STATUSQ_BUILD_PATH) \
--target StatusQ \
--config Release
-DCMAKE_BUILD_TYPE=Release \
--config Release \
$(HANDLE_OUTPUT)

statusq-install: | statusq-build
Expand Down Expand Up @@ -322,18 +322,29 @@ $(DOTHERSIDE): | deps
-DENABLE_DOCS=OFF \
-DENABLE_TESTS=OFF \
.. $(HANDLE_OUTPUT) && \
$(DOTHERSIDE_BUILD_CMD)
$(DOTHERSIDE_BUILD_CMD) \
$(HANDLE_OUTPUT)

dotherside: $(DOTHERSIDE)

dotherside-clean:
$(MAKE) -C vendor/DOtherSide/build --no-print-directory clean

STATUSGO := vendor/status-go/build/bin/libstatus.$(LIBSTATUS_EXT)
STATUSGO_LIBDIR := $(shell pwd)/$(shell dirname "$(STATUSGO)")
export STATUSGO_LIBDIR

status-go: $(STATUSGO)
Copy link
Contributor

Choose a reason for hiding this comment

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

@igor-sirotin I would keep this line instead of line 343, it's clearer and more intuitive.

Copy link
Contributor Author

@igor-sirotin igor-sirotin Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really care actually, just wanted to group all status-go-* targets together
If you're of strong opinion, I'm ok to revert it. Or we can wait for someone else to reply here 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

There is already STATUSGO defined in the line 333 because of that it's (at least to me) clearer to remove line 342 and have line 337 like status-go: | deps, but let's hear form others.

Copy link
Contributor Author

@igor-sirotin igor-sirotin Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@saledjenic, sorry, I think I don't fully understand you.
And the line numbering has also changed here 😄 I suppose you refer to the new one.

The targets we have

  1. Define the variable with binary path

    status-desktop/Makefile

    Line 333 in 66d2a2b

    STATUSGO := vendor/status-go/build/bin/libstatus.$(LIBSTATUS_EXT)

  2. Define the target, which is the status-go binary, which path is defined in1
    Also set the target dependencies and instructions to build the target.

    status-desktop/Makefile

    Line 337 in 66d2a2b

    $(STATUSGO): | deps

  3. Define a status-go target
    This is just an alias to use like make status-go, which is much easier than make vendor/status-go/build/bin/libstatus.dylib

    status-desktop/Makefile

    Line 342 in 66d2a2b

    status-go: $(STATUSGO)

  4. Define a status-go-clean target
    So I could write make status-go-clean instead of rm vendor/status-go/build/bin/libstatus.dylib

    status-desktop/Makefile

    Line 344 in 66d2a2b

    status-go-clean:

Note

From your first comment I though you want to leave the previous order of the targets. Like this:

  • status-go
  • $(STATUSGO)

But from you second comment do you propose to remove one of the targets? Did I get it right?

Copy link
Contributor

Choose a reason for hiding this comment

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

minor thing, irrelevant, just proceed and merge it

$(STATUSGO): | deps
echo -e $(BUILD_MSG) "status-go"
+ cd vendor/status-go && \
$(MAKE) statusgo-shared-library $(STATUSGO_MAKE_PARAMS) $(HANDLE_OUTPUT)

status-go: $(STATUSGO)

status-go-clean:
echo -e "\033[92mCleaning:\033[39m status-go"
rm -f $(STATUSGO)

STATUSKEYCARDGO := vendor/status-keycard-go/build/libkeycard/libkeycard.$(LIBSTATUS_EXT)
STATUSKEYCARDGO_LIBDIR := $(shell pwd)/$(shell dirname "$(STATUSKEYCARDGO)")
export STATUSKEYCARDGO_LIBDIR
Expand All @@ -349,7 +360,7 @@ QRCODEGEN := vendor/QR-Code-generator/c/libqrcodegen.a
$(QRCODEGEN): | deps
echo -e $(BUILD_MSG) "QR-Code-generator"
+ cd vendor/QR-Code-generator/c && \
$(MAKE) $(QRCODEGEN_MAKE_PARAMS)
$(MAKE) $(QRCODEGEN_MAKE_PARAMS) $(HANDLE_OUTPUT)

FLEETS := fleets.json
$(FLEETS):
Expand Down Expand Up @@ -713,9 +724,8 @@ pkg-windows: check-pkg-target-windows $(STATUS_CLIENT_EXE)

zip-windows: check-pkg-target-windows $(STATUS_CLIENT_7Z)

clean: | clean-common statusq-clean
rm -rf bin/* node_modules bottles/* pkg/* tmp/* $(STATUSGO) $(STATUSKEYCARDGO)
+ $(MAKE) -C vendor/DOtherSide/build --no-print-directory clean
clean: | clean-common statusq-clean status-go-clean dotherside-clean
rm -rf bin/* node_modules bottles/* pkg/* tmp/* $(STATUSKEYCARDGO)
+ $(MAKE) -C vendor/QR-Code-generator/c/ --no-print-directory clean

clean-git:
Expand Down
3 changes: 3 additions & 0 deletions ui/StatusQ/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ find_package(Qt5 COMPONENTS
add_subdirectory(../../vendor/SortFilterProxyModel SortFilterProxyModel)
add_subdirectory(../../vendor/qzxing/src qzxing)

target_compile_options(qzxing PRIVATE -w)
target_compile_options(SortFilterProxyModel PRIVATE -w)

### StatusQ library
### TODO: Move to a subdirectory for readability and better structure

Expand Down
1 change: 0 additions & 1 deletion ui/StatusQ/src/statusq.qrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,5 @@
<file>StatusQ/Controls/StatusLinkText.qml</file>
<file>StatusQ/Core/Utils/ModelChangeGuard.qml</file>
<file>StatusQ/Core/Utils/StackViewStates.qml</file>
<file>StatusQ/Components/StatusDraggableListItem.qml</file>
</qresource>
</RCC>