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

Build tags assigned incorrectly ("netgo ledger,") with Make 4.3+ #2017

Closed
5 tasks
sigv opened this issue Jan 14, 2023 · 7 comments · Fixed by #2018
Closed
5 tasks

Build tags assigned incorrectly ("netgo ledger,") with Make 4.3+ #2017

sigv opened this issue Jan 14, 2023 · 7 comments · Fixed by #2018
Assignees

Comments

@sigv
Copy link
Contributor

sigv commented Jan 14, 2023

Summary of Bug

Appending More Text to Variables is done in Makefile when working with build tags.
Something changed in Make 4.3 regarding empty values being appended to themselves using the += operator.

Ubuntu 20.04 LTS ships with Make 4.2 and results in build tags "netgo,ledger".
Ubuntu 22.04 LTS ships with Make 4.3 and results in build tags "netgo ledger,".

Quoting the Make documentation:

Using ‘+=’ is similar to:

objects = main.o foo.o bar.o utils.o
objects := $(objects) another.o

but differs in ways that become important when you use more complex values.

Version

Any project version, when building with Make 4.3 or Make 4.4.

Steps to Reproduce

Tested on Ubuntu 22.04.1, however should apply to any distro/version as we are building upstream Make for test:

sudo apt -qy install make gcc

WORKSPACE=$(mktemp -d --tmpdir make.XXX)
cd "$WORKSPACE"

# Create minimal Makefile based on logic in repository

cat >Makefile <<EOF
build_tags = netgo
build_tags += ledger

whitespace :=
whitespace += \$(whitespace)
comma := ,
build_tags_comma_sep := \$(subst \$(whitespace),\$(comma),\$(build_tags))

.PHONY: all
all:
$(echo -e '\t')@echo '"\$(build_tags_comma_sep)"'
EOF

# Additional Makefile that uses explicit assignment

cp Makefile Makefile.proposal
sed -i 's/^whitespace += \$(whitespace)$/whitespace := $(whitespace) $(whitespace)/' Makefile.proposal

# Build relevant Make versions for testing

wget -nv https://ftp.gnu.org/gnu/make/make-4.2.tar.gz && tar -xf make-4.2.tar.gz
# Make 4.2 does not play nice with newer releases. Needs a patch to check "newer or equal" instead of just "equal".
sed -i 's/^# if _GNU_GLOB_INTERFACE_VERSION == GLOB_INTERFACE_VERSION$/# if _GNU_GLOB_INTERFACE_VERSION >= GLOB_INTERFACE_VERSION/' make-4.2/glob/glob.c
(cd make-4.2 && ./configure && make)

wget -nv https://ftp.gnu.org/gnu/make/make-4.3.tar.gz && tar -xf make-4.3.tar.gz
(cd make-4.3 && ./configure && make)

wget -nv https://ftp.gnu.org/gnu/make/make-4.4.tar.gz && tar -xf make-4.4.tar.gz
(cd make-4.4 && ./configure && make)

# Try to use the Makefiles.

make-4.2/make -f Makefile
# "netgo,ledger"
make-4.3/make -f Makefile
# "netgo ledger,"
make-4.4/make -f Makefile
# "netgo ledger,"

make-4.2/make -f Makefile.proposal
# "netgo,ledger"
make-4.3/make -f Makefile.proposal
# "netgo,ledger"
make-4.4/make -f Makefile.proposal
# "netgo,ledger"

cd
rm -rf "$WORKSPACE"

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
  • Is a spike necessary to map out how the issue should be approached?
sigv added a commit to sigv/gaia that referenced this issue Jan 14, 2023
With Make 4.3+, the empty whitespace does not seem to work as
originally intended. This causes build tags to be "netgo ledger,"
on Ubuntu 22.04 and other systems that include the newer Make
version. The build tags were intended as "netgo,ledger" which
can be observed on Make 4.2 (shipped with Ubuntu 20.04).

This change swaps out the `+=` use in favor of an explicit `:=`.
Ref: https://www.gnu.org/software/make/manual/html_node/Appending.html

Closes cosmos#2017.
@sigv
Copy link
Contributor Author

sigv commented Jan 14, 2023

It looks like Gaia v7.1.0 was built with Make 4.3+ as well:

$ ./gaiad-v7.1.0-linux-amd64 version --long 2>&1 | head -n5
name: gaia
server_name: gaiad
version: v7.1.0
commit: 5db8fcc9a229730f5115bed82d0f85b6db7184b4
build_tags: netgo ledger,

@mpoke mpoke added this to Cosmos Hub Jan 20, 2023
@github-project-automation github-project-automation bot moved this to 🩹 Triage in Cosmos Hub Jan 20, 2023
@mpoke mpoke moved this from 🩹 Triage to 🏗 In progress in Cosmos Hub Jan 20, 2023
@mpoke mpoke moved this from 🏗 In progress to 👀 In review in Cosmos Hub Jan 20, 2023
@yaruwangway
Copy link
Contributor

Hi @sigv, thank you for opening this issue, can you check if this PR fixes what you addressed in this issue ? #2051

@yaruwangway
Copy link
Contributor

yaruwangway commented Jan 20, 2023

ha, sry, just saw you already opened pr, will review! @sigv

@sigv
Copy link
Contributor Author

sigv commented Jan 20, 2023

ha, sry, just saw you already opened pr, will review!

No worries! And thank you!

Some background:

We (kjnodes) discovered this with lavanet/[email protected], where the -X flag was not quoted, resulting in build failure. Most derivate Cosmos-based chains appear to have copy-pasted the -X flag quoted, as visible in cosmos/[email protected]. This enables spaces in the $(build_tags_comma_sep) to build properly. From my understanding, this doesn't directly affect any operational aspect of the node itself, therefore backporting is entirely optional.

As for the proposed change itself, we proposed it to Lava Network and it was released in lavanet/[email protected]. It gets the job done. The test scenario to do is just spin up a fresh VM on 20.04 and 22.04, run builds with and without patch, compare output of version subcommand from the built binary. Looks reasonable to me.

Let me know if I can help with anything else!

@sigv
Copy link
Contributor Author

sigv commented Feb 16, 2023

@yaruwangway, any chance of having the change reviewed?

@yaruwangway
Copy link
Contributor

Hi @sigv , i reviewed, all good, but we need two reviewers to get merged, i will ask my colleagues.

@sigv
Copy link
Contributor Author

sigv commented Feb 16, 2023

Much appreciated! 🙏🏻🥂

@yaruwangway yaruwangway mentioned this issue Feb 17, 2023
8 tasks
yaruwangway pushed a commit that referenced this issue Feb 20, 2023
With Make 4.3+, the empty whitespace does not seem to work as
originally intended. This causes build tags to be "netgo ledger,"
on Ubuntu 22.04 and other systems that include the newer Make
version. The build tags were intended as "netgo,ledger" which
can be observed on Make 4.2 (shipped with Ubuntu 20.04).

This change swaps out the `+=` use in favor of an explicit `:=`.
Ref: https://www.gnu.org/software/make/manual/html_node/Appending.html

Closes #2017.

Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Milan Mulji <[email protected]>
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Cosmos Hub Feb 20, 2023
mergify bot pushed a commit that referenced this issue Feb 20, 2023
With Make 4.3+, the empty whitespace does not seem to work as
originally intended. This causes build tags to be "netgo ledger,"
on Ubuntu 22.04 and other systems that include the newer Make
version. The build tags were intended as "netgo,ledger" which
can be observed on Make 4.2 (shipped with Ubuntu 20.04).

This change swaps out the `+=` use in favor of an explicit `:=`.
Ref: https://www.gnu.org/software/make/manual/html_node/Appending.html

Closes #2017.

Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Milan Mulji <[email protected]>
(cherry picked from commit 297cdb9)
mergify bot pushed a commit that referenced this issue Feb 20, 2023
With Make 4.3+, the empty whitespace does not seem to work as
originally intended. This causes build tags to be "netgo ledger,"
on Ubuntu 22.04 and other systems that include the newer Make
version. The build tags were intended as "netgo,ledger" which
can be observed on Make 4.2 (shipped with Ubuntu 20.04).

This change swaps out the `+=` use in favor of an explicit `:=`.
Ref: https://www.gnu.org/software/make/manual/html_node/Appending.html

Closes #2017.

Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Milan Mulji <[email protected]>
(cherry picked from commit 297cdb9)
mergify bot pushed a commit that referenced this issue Feb 20, 2023
With Make 4.3+, the empty whitespace does not seem to work as
originally intended. This causes build tags to be "netgo ledger,"
on Ubuntu 22.04 and other systems that include the newer Make
version. The build tags were intended as "netgo,ledger" which
can be observed on Make 4.2 (shipped with Ubuntu 20.04).

This change swaps out the `+=` use in favor of an explicit `:=`.
Ref: https://www.gnu.org/software/make/manual/html_node/Appending.html

Closes #2017.

Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Milan Mulji <[email protected]>
(cherry picked from commit 297cdb9)
mmulji-ic pushed a commit that referenced this issue Feb 20, 2023
With Make 4.3+, the empty whitespace does not seem to work as
originally intended. This causes build tags to be "netgo ledger,"
on Ubuntu 22.04 and other systems that include the newer Make
version. The build tags were intended as "netgo,ledger" which
can be observed on Make 4.2 (shipped with Ubuntu 20.04).

This change swaps out the `+=` use in favor of an explicit `:=`.
Ref: https://www.gnu.org/software/make/manual/html_node/Appending.html

Closes #2017.

Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Milan Mulji <[email protected]>
(cherry picked from commit 297cdb9)

Co-authored-by: Valters Jansons <[email protected]>
mmulji-ic pushed a commit that referenced this issue Feb 20, 2023
With Make 4.3+, the empty whitespace does not seem to work as
originally intended. This causes build tags to be "netgo ledger,"
on Ubuntu 22.04 and other systems that include the newer Make
version. The build tags were intended as "netgo,ledger" which
can be observed on Make 4.2 (shipped with Ubuntu 20.04).

This change swaps out the `+=` use in favor of an explicit `:=`.
Ref: https://www.gnu.org/software/make/manual/html_node/Appending.html

Closes #2017.

Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Milan Mulji <[email protected]>
(cherry picked from commit 297cdb9)

Co-authored-by: Valters Jansons <[email protected]>
yaruwangway added a commit that referenced this issue Mar 1, 2023
* Update changelog for v9 rc7 (#2219)

* Update v8.0.1 changelog (#2221)

* Consistently generate build tags on newer Make (#2018)

With Make 4.3+, the empty whitespace does not seem to work as
originally intended. This causes build tags to be "netgo ledger,"
on Ubuntu 22.04 and other systems that include the newer Make
version. The build tags were intended as "netgo,ledger" which
can be observed on Make 4.2 (shipped with Ubuntu 20.04).

This change swaps out the `+=` use in favor of an explicit `:=`.
Ref: https://www.gnu.org/software/make/manual/html_node/Appending.html

Closes #2017.

Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Milan Mulji <[email protected]>

* - Update doc to follow Go conventions
- Improve code readability

* update params

* docs: update docs

* fix: lint

---------

Co-authored-by: lg <[email protected]>
Co-authored-by: Valters Jansons <[email protected]>
Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Milan Mulji <[email protected]>
Co-authored-by: Simon Noetzlin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants