Fix iverilog files not update bug during overwrite installation

[vowstar]

The most original detailed information is here: https://bugs.gentoo.org/705412

Detailed Description:

The original Makefile used a very unusual installation method, e.g.,

https://github.com/steveicarus/iverilog/blob/master/Makefile.in#L356

$(includedir)/vpi_user.h: $(srcdir)/vpi_user.h installdirs
        $(INSTALL_DATA) $(srcdir)/vpi_user.h "$(DESTDIR)$(includedir)/vpi_user.h"

It checks $(includedir)/vpi_user.h but installed to "$(DESTDIR)$(includedir)/vpi_user.h"

If an old version of iverilog is already installed, when download a new released compressed package and decompress it, the timestamp of source code may old and when doing overwrite installation, the old file may not update due to the installed file have new timestamp.

This causes some problems, which can happen in the following cases:

• bump version form source tar ball
• reinstallation form source tar ball

I'm thinking about how to fix it, firstly I've tried update the timestamp of all source

find . -exec touch {} +

It success but too dirty, I don't want recompile every time and it may cause other problem.

Also I'm thinking about change code like this:

e.g.,
$(includedir)/vpi_user.h: $(srcdir)/vpi_user.h installdirs
        $(INSTALL_DATA) $(srcdir)/vpi_user.h "$(DESTDIR)$(includedir)/vpi_user.h"
changed to
$(DESTDIR)$(includedir)/vpi_user.h: $(srcdir)/vpi_user.h installdirs
        $(INSTALL_DATA) $(srcdir)/vpi_user.h "$(DESTDIR)$(includedir)/vpi_user.h"

But it only fix the problem on gentoo Linux, In general the problem exist.

I also think about how to install by checksum files, but Makefile can't beautifully do it.

Add .PHONY to install also too weird.

So, I don't use the above method.

In order to install correctly every time, the installation (INSTALL_DATA/INSTALL_PROGRAM/INSTALL_SCRIPT) should not just check the results based on timestamp, Instead, perform a true overwrite installation to ensure that the program behaves correctly.

Even if I install the new version first and then overwrite installation the old version of the iverilog, It is unreasonable not to install it due to timestamp is old.

I changed 20 Makefile.in files and use traditional method to do INSTALL_DATA/INSTALL_PROGRAM/INSTALL_SCRIPT, by the way fix some hidden parallel compilation problem, improve the maintainability of the installation script to make it look simpler.

---

• Fix bug: https://bugs.gentoo.org/705412
• Fix bug: sci-electronics/iverilog: bump to 10.3 gentoo/gentoo#14096
• Related: Makefile.in: fix mkdir race during "make -j N install" #294

[xdch47]

I think prefixing is enough to fix the problem.
"all makefiles" basically work like that, which is fine for the packaging or installation within a git environment. so IMO this fix is overcomplicated...

the only case, where it will not work, would be a tar-based source installation in linux from scratch.
and in that case "make uninstall" should ran before, otherwise there will be the risk that orphaned files are staying in the system.

By the way, to fix the race-condition with the install directory, I would you order-only-prerequisite types like

$(INSTALL_TARGET): $(FILE) | $(INSTALL_DIR)
    install .....

--> have a look here:
https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html

[minux]

prefixing targets with $(DESTDIR) won't work for tarball based source installation, and make uninstall before make install is also problematic.

Consider this timestamp sequence:
release A: time t1,
release A+1: time t2, t2 > t1

User at time t3 (t3 > t2) installs release A, but then found out release A+1 is out too, so it
installs A+1. Because all installed files is at t3, none of the rule will be triggered, and the
user will get outdated files on disk.

It's true that if the user runs make uninstall will likely fix this issue, but the problem is that,
to be able to fully uninstall all files, the user can't just run make uninstall from release A+1,
they have to run make uninstall from release A. This creates extra work without much
benefits for the user.

I don't think prefixing $(DESTDIR) is how "all makefiles" basically works. (Nearly) all makefiles override all installations files unconditionally. And this is very different from prefixing $(DESTIDR) on the target path.

Of course, one way to solve the specific problem I mentioned with prefixing $(DESTDIR) would be to use install -p to preserve the timestamp. But then when the user wants to downgrade from release A+1 to release A, it still fails to update the files.

The takeaway is that: the install target should override all target files unconditionally.

[xdch47]

as I stated before a lot of makefiles that i have seen will break in the scenario you mentioned.

you'll use a package manager (or how would you be able to remove orphaned files?), or use a repository.
the $(DESTDIR) variable is (nearly) always only there for packaging, since otherwise it will break the library path.

.PHONY: would be the way to force it and some people do that.
however from my point of view, the specified target should be created by the rule (or the rule should be phony) and this is here not case...
I think your installdirs fix (7de00ee) is worse, than arguing that prefixing with $(DESTDIR) is not correct, because the timestamps on directories change whenever a file is added, removed, or renamed and we certainly don’t want to rebuild all the targets whenever the directory’s timestamp changes.

[minux]

DESTDIR is only used for packaging, I agree. But with your proposed change, regular `make install` without DESTDIR will still exhibit incorrect behavior for the cases I showed. (Showing that there exists other incorrectly written Makefiles doesn't mean we have to join them, especially if there is a correct way to handle those cases.) The only correct way is to *unconditionally* overwrite all destination files while installing. There is no easy way to remove orphaned files, and the only reliable way to remove them is to run `make uninstall` from the *original* version. Hopefully the newly installed files should overwrite all existing files. The installdirs fix could use order-only prerequisites, but I believe for this specific case, the difference doesn't matter in practice. (How could my change force rebuild all targets when directory timestamp is changed? It will only force reinstallation of the files when you run `make install` again, but that is *exactly* what you want when you do `make install`.)

[xdch47]

Again, for a pedantic and 100% correct solution .PHONY would be the best.
For readability and rea-world, i think prefixing is the nicest.

Let's have a look at "real world" examples:

1. You use a linux distro with building software from source (e.g. arch/AUR, gentoo linux)
2. you have a design cluster and install different iverilog version in separate directories using
   enviroment modules (--> modifies LD_LIBRARY_PATH) to chose a specific version. - the default version is installed within the system.

both will fail at the current state and work with $(DESTDIR).
All I vote for is a pragmatic & simple solution. 20 commits seems a lot and it does not got simpler, but more error prone …

[minux]

The size of the PR seems large is only because each file is split into each own commit. Either of your proposed .PHONY or $(DESTDIR)-prefixing solutions will still change all 20 Makefiles, so with the one-file-per-commit organization, it will also result in 20 commits. Thus your point of simpler solution (in terms of commit count) doesn't hold. Also, your list of real-world examples conveniently omit one of the more common usage scenarios where prefixing $(DESTDIR) fails. 3. the user regularly downloads source tarball and manually installs (updates) into /usr/local/. If we have to fix the problem, let's fix it completely this time, not partially. And for 3, the problem with $(DESTDIR)-prefixing fix will lead to very subtle problems for the user (some of the headers file are silently not updated.) The .PHONY solution is ok in this regard, but the only benefit it offers is that it enables parallel installation of multiple targets, and it's slightly simpler. The reason I don't like .PHONY is that it's completely negates the benefit of providing the dependency information to make, and it's a unconventional usage of .PHONY, which is supposedly used to label targets that are not actually files/directories (like clean, install and installdirs). If make doesn't need the dependency for each installed file, then just don't provide it and list all installed files of the same kind under the recipe for each kind. The current PR is larger but is modeled after automake generated Makefiles, and it represents a generally accepted approach to this problem. The other reason against .PHONY is that, we will need to include a comment explaining why we provide .POHNY for a file that actually exists, and it adds more cognitive load to future maintainers, otherwise, the maintainer might forget to add a new install file to .PHONY and reintroduce this problem. That's why I think .PHONY solution, while correct, is actually more error prone that the proposed approach. (Just imagine if you want to add to add a new installed file: The .PHONY solution requires you to add one more rule and also one more .PHONY declaration, but with this change, you just change the dependency of the appropriate rule and add one more line to its recipe. Which one is easier and less error prone? Not only is the 2nd one simpler, it also has less opportunity for errors.)

[minux]

To summarize: If it's for a local patch in Gentoo portage, .PHONY solution would be the best, as it minimizes diff size and we can afford to include comments explaining this reason why .PHONY is needed. But for the upstream, we need a solution that is not only 100% correct but also with the least cognitive load and least error-prone for future changes.

[vowstar]

I think prefixing is enough to fix the problem.
"all makefiles" basically work like that, which is fine for the packaging or installation within a git environment. so IMO this fix is overcomplicated...

the only case, where it will not work, would be a tar-based source installation in linux from scratch.
and in that case "make uninstall" should ran before, otherwise there will be the risk that orphaned files are staying in the system.

By the way, to fix the race-condition with the install directory, I would you order-only-prerequisite types like

$(INSTALL_TARGET): $(FILE) | $(INSTALL_DIR)
    install .....

--> have a look here:
https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html

Thanks for the detailed explanation,

I also consider that if not fixed, even when compiling with git source code, assuming a distributed scenario of shared file system, this problem still exists.
If user A compiles and installs with git source code at first. User B modifies the source code, and then compiles and installs, it may fail (the file is not updated), unless you first execute make uninstall and ensure that they uninstall the same version file correctly (of course this is likely to imply consistency issues).

So the installation process must be performed exactly, rather than checking the timestamp.

order-only-prerequisite is nice to resolve parallel installation dependency problem, the current code is solved using another way.

e.g.:

install: obj_b
	@echo C
obj_b: obj_a
	@echo B
obj_a:
	@echo A

The order is A, and then B, C

In this parallel install fix, installdirs has executed exactly before installfiles, so no need apply order-only-prerequisite.

[martinwhitaker]

I think this is one for Steve to decide.

[steveicarus]

I think the original premise of this issue may be mistaken. In particular, the install rule was standard practice for a very long time. The only obvious issue, though, is the absence of $(DESTDIR) in the target rule. I can see that being fixed. (DESTDIR is only used when installing to a fake root, or something similar.)

I haven't looked at the commits yet, but that's my brief thinking.

[minux]

As mentioned earlier, prefixing $(DESTDIR) will only fix the case when $(DESTDIR) is specified and $(DESTDIR) points to an empty tree. And it will break for the regular "make; make install" scenario (when updating an previous installation). (see #300 (comment) for an example.) When installing, we must not consider the timestamp of the target files and must overwrite them unconditionally.

[steveicarus]

I approve in the sense that I'm not opposed to these changes. The make files as they are have always worked fine for me, even when making packages, but if this change makes the makefiles more robust, then OK then. So long as the changes don't add a lot of complexity.

[steveicarus]

Can everyone on this conversation review and approve the sum total of the changes? I reviewed the changes, and to my eye, they seem OK. There has been some debate, I know, but this issue hasn't impacted me.

To review, go the "Files Changed" tab at the top of this pull request. The green "Review Changes" button would be a great place to submit the review, and explicitly approve the pull request.

Thanks, all.

[vowstar]

Can everyone on this conversation review and approve the sum total of the changes? I reviewed the changes, and to my eye, they seem OK. There has been some debate, I know, but this issue hasn't impacted me.

To review, go the "Files Changed" tab at the top of this pull request. The green "Review Changes" button would be a great place to submit the review, and explicitly approve the pull request.

Thanks, all.

Thank you!

I tried approve but found that the submitter was unable to approve their own code.
I compiled and tested these commits in a clean environment with fake timestamp and real timestamp.

[martinwhitaker]

Testing this with an out-of-tree build gives the following failure:

make[1]: Entering directory '/pack/icarus/build/tgt-stub'
../../source/tgt-stub/../mkinstalldirs "/usr/local/lib/ivl"
/usr/bin/install -c ./stub.tgt "/usr/local/lib/ivl/stub.tgt"
/usr/bin/install -c -m 644 ./stub.conf "/usr/local/lib/ivl/stub.conf"
/usr/bin/install: cannot stat './stub.conf': No such file or directory
make[1]: *** [Makefile:93: installfiles] Error 1
make[1]: Leaving directory '/pack/icarus/build/tgt-stub'
make: *** [Makefile:333: install] Error 2

[vowstar]

Testing this with an out-of-tree build gives the following failure:

make[1]: Entering directory '/pack/icarus/build/tgt-stub'
../../source/tgt-stub/../mkinstalldirs "/usr/local/lib/ivl"
/usr/bin/install -c ./stub.tgt "/usr/local/lib/ivl/stub.tgt"
/usr/bin/install -c -m 644 ./stub.conf "/usr/local/lib/ivl/stub.conf"
/usr/bin/install: cannot stat './stub.conf': No such file or directory
make[1]: *** [Makefile:93: installfiles] Error 1
make[1]: Leaving directory '/pack/icarus/build/tgt-stub'
make: *** [Makefile:333: install] Error 2

It is very weird, It build success in my environment. I list the iverilog/tgt-stub, and stub.conf exists.

iverilog/tgt-stub
|-- Makefile.in
|-- classes.c
|-- constant.c
|-- cppcheck.sup
|-- enumerate.c
|-- expression.c
|-- priv.h
|-- statement.c
|-- stub-s.conf
|-- stub.c
|-- stub.conf
|-- switches.c
`-- types.c

Which branch or package are you using?
I want to reproduce.

[vowstar]

I build a clean environment using docker whith debian:bullseye to test the iverilog build, and it build success both in gentoo and debian.

The Dockerfile can build at any amd64 hostmachine with docker. It clone and checkout the branch in this PR, build and test.

FROM debian:bullseye

LABEL maintainer="vowstar"

# Update and install base software
RUN DEBIAN_FRONTEND=noninteractive apt-get update -qq && \
    DEBIAN_FRONTEND=noninteractive apt-get install -yq \
        build-essential \
        autoconf \
        gperf \
        bison \
        flex \
        wget \
        curl \
        git \
        python \
        perl \
        sudo

# Fetch
RUN mkdir /build && \
    cd /build && \
    git clone --progress --recursive https://github.com/vowstar/iverilog.git && \
    git clone --progress --recursive https://github.com/steveicarus/ivtest.git && \
    cd /build/iverilog && \
    git checkout fix-makefile

# Build
RUN cd /build/iverilog && \
    sh autoconf.sh && \
    ./configure && \
    make && \
    make install

# Test
RUN cd /build/ivtest && \
    perl ./vvp_reg.pl

Build with command: docker build . -t iverilog-debian

The build and test log is: https://pastebin.com/5kxKkn1w

The line 2035 (https://pastebin.com/5kxKkn1w) shows that /usr/local/lib/ivl/stub.conf has been successfully installed.

make[1]: Entering directory '/build/iverilog/tgt-stub'
./../mkinstalldirs "/usr/local/lib/ivl"
/usr/bin/install -c ./stub.tgt "/usr/local/lib/ivl/stub.tgt"
/usr/bin/install -c -m 644 ./stub.conf "/usr/local/lib/ivl/stub.conf"
/usr/bin/install -c -m 644 ./stub-s.conf "/usr/local/lib/ivl/stub-s.conf"
make[1]: Leaving directory '/build/iverilog/tgt-stub'

[vowstar]

Apart from Debian and Gentoo, there is no problem building on CentOS8.

The CentOS8 build log (build success):

https://pastebin.com/rjwp2mAj

The docker file:

FROM centos:8

LABEL maintainer="vowstar"

# Update and install base software
RUN dnf -y update && \
    dnf -y groupinstall 'Development Tools' && \
    dnf -y --enablerepo=PowerTools install \
        autoconf \
        gperf \
        bison \
        flex \
        wget \
        curl \
        git \
        python36 \
        perl \
        sudo

# Fetch
RUN mkdir /build && \
    cd /build && \
    git clone --progress --recursive https://github.com/vowstar/iverilog.git && \
    git clone --progress --recursive https://github.com/steveicarus/ivtest.git && \
    cd /build/iverilog && \
    git checkout fix-makefile

# Build
RUN cd /build/iverilog && \
    sh autoconf.sh && \
    ./configure && \
    make && \
    make install

# Test
RUN cd /build/ivtest && \
    perl ./vvp_reg.pl

[martinwhitaker]

You need to try an out-of-tree build. Try this:

mkdir iverilog
cd iverilog
git clone https://github.com/vowstar/iverilog.git source
cd source
sh autoconf.sh
cd ..
mkdir build
cd build
../source/configure
make -j8
make install

[vowstar]

You need to try an out-of-tree build. Try this:

mkdir iverilog
cd iverilog
git clone https://github.com/vowstar/iverilog.git source
cd source
sh autoconf.sh
cd ..
mkdir build
cd build
../source/configure
make -j8
make install

@martinwhitaker Thank you for detailed explanation , I already understand what happened.
When installing tgt-stub and tgt-vhdl, due to the wrong target path, the install scripts can't find the file to be installed. The missing file is: stub.conf, stub-s.conf, vhdl.conf, vhdl-s.conf

I rebased the code to latest and changed to right path fixed this problem: 969426a

This problem may also be reproduced on other branches.

Thanks @minux and @xdch47 for helping fix these issues, I applied your solution in these commits.

Dockerfile for testing an out-of-tree build:

FROM debian:bullseye

LABEL maintainer="vowstar"

# Update and install base software
RUN DEBIAN_FRONTEND=noninteractive apt-get update -qq && \
    DEBIAN_FRONTEND=noninteractive apt-get install -yq \
        build-essential \
        autoconf \
        gperf \
        bison \
        flex \
        wget \
        curl \
        git \
        python \
        perl \
        sudo

# Fetch
RUN mkdir /workspace && \
    cd /workspace && \
    git clone --progress --recursive https://github.com/vowstar/iverilog.git && \
    git clone --progress --recursive https://github.com/steveicarus/ivtest.git && \
    cd /workspace/iverilog && \
    git checkout fix-makefile

# Build
RUN cd /workspace/iverilog && \
    sh autoconf.sh && \
    mkdir /workspace/build && \
    cd /workspace/build && \
    ../iverilog/configure && \
    make -j && \
    make -j install

# Test
RUN cd /workspace/ivtest && \
    perl ./vvp_reg.pl

Test result: Build and install success, the test log is:

https://pastebin.com/wYjj15xv

Test results:
  Total=2441, Passed=2433, Failed=8, Not Implemented=0, Expected Fail=0

[vowstar]

Do we have any update?

[steveicarus]

Sorry, new day job. Merged this pull request.

…

On Sat, Feb 29, 2020 at 12:54 AM Rui Huang ***@***.***> wrote: Do we have any update? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#300?email_source=notifications&email_token=AAACKMREBGYDDUM52ETGEPLRFDGKXA5CNFSM4KIR6QG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENLUIRY#issuecomment-592921671>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACKMUYGZYTVKKSN7OTIMDRFDGKXANCNFSM4KIR6QGQ> .

-- Steve Williams "The woods are lovely, dark and deep. steve@icarus.com <steveicarus@gmail.com> But I have promises to keep, http://www.icarus.com and lines to code before I sleep, http://www.picturel.com And lines to code before I sleep."