-
Notifications
You must be signed in to change notification settings - Fork 79
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(makefile): Bugfixes and minor improvements #10423
Conversation
Jenkins Builds
|
|
||
STATUSGO := vendor/status-go/build/bin/libstatus.$(LIBSTATUS_EXT) | ||
STATUSGO_LIBDIR := $(shell pwd)/$(shell dirname "$(STATUSGO)") | ||
export STATUSGO_LIBDIR | ||
|
||
status-go: $(STATUSGO) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
-
Define the variable with binary path
Line 333 in 66d2a2b
STATUSGO := vendor/status-go/build/bin/libstatus.$(LIBSTATUS_EXT) -
Define the target, which is the status-go binary, which path is defined in
1
Also set the target dependencies and instructions to build the target.
Line 337 in 66d2a2b
$(STATUSGO): | deps -
Define a
status-go
target
This is just an alias to use likemake status-go
, which is much easier thanmake vendor/status-go/build/bin/libstatus.dylib
Line 342 in 66d2a2b
status-go: $(STATUSGO) -
Define a
status-go-clean
target
So I could writemake status-go-clean
instead ofrm vendor/status-go/build/bin/libstatus.dylib
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?
There was a problem hiding this comment.
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
aa87e84
to
66d2a2b
Compare
What does the PR do
Makefile:
StatusQ
build outputQrCodeGenerator
build outputStatusQ
cmake warningsclean
command forDOtherSide
andstatus-go
Other:
SortFilterProxyModel
andqzxing
warningsstatusq.qrc
Affected areas
Makefile, build system