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
build: add all subservers to dev build with an optional with-rpc build param #4380
Conversation
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.
Concept ACK
make/testing_flags.mk
Outdated
@@ -1,8 +1,14 @@ | |||
DEV_TAGS = dev | |||
RPC_TAGS = chainrpc walletrpc signrpc invoicesrpc autopilotrpc watchtowerrpc |
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.
IIRC there are a few other subservers also, should we just enable them?
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.
The original idea was to only include the subservers that are also present in the release build, but actually I think it makes sense to include all.
This also changes what we itest, but imho it is more correct to itest on the full build and have a smaller subset for release than itest on the smaller subset. PTAL
ping @cfromknecht |
@@ -61,6 +67,6 @@ backend = btcd | |||
endif | |||
|
|||
# Construct the integration test command with the added build flags. | |||
ITEST_TAGS := $(DEV_TAGS) rpctest chainrpc walletrpc signrpc invoicesrpc autopilotrpc watchtowerrpc $(backend) | |||
ITEST_TAGS := $(DEV_TAGS) $(RPC_TAGS) rpctest $(backend) |
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.
won't this result in duplicating the tags? or is the assumption that you don't need with-rpc
for itest compilation?
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.
My original intention was to allow creating a dev build which includes all RPC subservers while we also keep backward compatibility with existing build targets. I'd expect with-rpc
to be used standalone and never as part of automated builds.
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.
Now reading it again, it could be I'm misunderstanding your comment though :)
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.
nope that answers my q, agree it doesn't make sense to use with-rpc
for itests so should be fine
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.
LGTM 💯
This PR enables having a dev build with all subservers with an optional
with-rpc=1
build param.