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

makefile: ignore errors during clean #36

Merged
merged 1 commit into from
Oct 30, 2022

Conversation

craftyguy
Copy link
Contributor

The file(s) being removed may not exist, but make shouldn't fail because of it:

    $ make clean
    rm ./firmware/*.uf2
    rm: cannot remove './firmware/*.uf2': No such file or directory
    make: *** [Makefile:10: clean] Error 1
    $ echo $?
    2

-f silences any output from rm even though the single dash tells make to ignore the failure, which just creates noise.

@craftyguy
Copy link
Contributor Author

maybe this is too aggressive, and -f should be removed in case the failure is legitimate (e.g. no write access to the file(s)?), what do y'all think?

@ghost
Copy link

ghost commented Oct 24, 2022

.PHONY should include all.

I suggest you remove - in -rm as you can get that effect by setting MAKEFLAGS=--silent in your environment, and seeing the output is the expected behavior.

Perfectly happy with the -f.

As we assume we are running in the top-level directory (which is reasonable) remove the ./ prefix. If you want to be able to run make from any directory then here is how to find the top-level directory TOP:=$(dir $(lastword $(MAKEFILE_LIST))) and you can use that to quality paths.

Do we need the $(PWD) in the firmware rules? If not remove those too.

Unrelated... does the .PHONY target with dependencies do anything with those dependencies? If not I suggest we remove them. podman build --tag zmk --file Dockerfile . takes ~1 second the 2nd time using cached values so I suggest we just eliminate the non-standard setup target and do the docker build in the firmware rule before the docker run. The big win was leaving the container around instead of rebuilding those each time and standard Makefile. What about adding a distclean target to remove the containers and images?

I will be adding a install target for my update script.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Update commit message to remove the "the single dash" part. I had a few other suggestions (above) that I suggest we just squeeze in here.

@craftyguy
Copy link
Contributor Author

craftyguy commented Oct 24, 2022

Do we need the $(PWD) in the firmware rules? If not remove those too.

Seems like it is, else the build fails because it can't find the board (presumably because config wasn't passed into the container?)

output from failed `make`
❯ make
/usr/bin/podman build --tag zmk --file Dockerfile .
STEP 1/8: FROM zmkfirmware/zmk-build-arm:stable
STEP 2/8: WORKDIR /app
--> Using cache bade143b745e858d2f65fd714ab001507307af52e1cdc7254b98fbd209ddecd4
--> bade143b745
STEP 3/8: COPY config/west.yml config/west.yml
--> Using cache be17afca292457eb802114b7aff70a9c4abb97e50ea45b0b7c861e76176cf8f0
--> be17afca292
STEP 4/8: RUN west init -l config
--> Using cache ba65ab0dc1008cb764c663dd42de67b5619590b57b93238a861b9cff181c3613
--> ba65ab0dc10
STEP 5/8: RUN west update
--> Using cache c860839014315c2b3b99c656b0b4c558607cc08497fb275f85946223f904cf0e
--> c8608390143
STEP 6/8: RUN west zephyr-export
--> Using cache 6dd04f480a311216d60bbd41a3e6bc1b4729c0c1e19b6a67563b6642951dbc74
--> 6dd04f480a3
STEP 7/8: COPY bin/build.sh ./
--> Using cache c4f467b0cb48c8b04602642c2a9e2242c617d9971ecee86686f005fbfae7eb32
--> c4f467b0cb4
STEP 8/8: CMD ["./build.sh"]
--> Using cache 12d3fe25915cf88ed90cf0225553e11803181299e80898cf078383ff2d0317bc
COMMIT zmk
--> 12d3fe25915
Successfully tagged localhost/zmk:latest
12d3fe25915cf88ed90cf0225553e11803181299e80898cf078383ff2d0317bc
echo /home/clayton/src/Adv360-Pro-ZMK
/home/clayton/src/Adv360-Pro-ZMK
/usr/bin/podman  run --rm -it --name zmk \
        -v firmware:/app/firmware \
        -v config:/app/config:ro \
        -e TIMESTAMP=20221024175615 \
        zmk
-- west build: generating a build system
Including boilerplate (Zephyr base): /app/zephyr/cmake/app/boilerplate.cmake
-- Application: /app/zmk/app
-- ZMK Config directory: /app/config
CMake Warning at cmake/ZephyrBuildConfig.cmake:199 (message):
  Failed to locate keymap file!
Call Stack (most recent call first):
  /app/zephyr/cmake/app/boilerplate.cmake:113 (find_package)
  /app/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:24 (include)
  /app/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:35 (include_boilerplate)
  CMakeLists.txt:15 (find_package)


-- Zephyr version: 3.0.0 (/app/zephyr)
-- Found Python3: /usr/bin/python3.8 (found suitable exact version "3.8.10") found components: Interpreter
-- Found west (found suitable version "0.12.0", minimum required is "0.7.1")
-- Board: adv360_left
No board named 'adv360_left' found.

Please choose one of the following boards:

arc:
  em_starterkit
<truncated>
CMake Error at /app/zephyr/cmake/app/boilerplate.cmake:388 (message):
  Invalid BOARD; see above.
Call Stack (most recent call first):
  /app/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:24 (include)
  /app/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:35 (include_boilerplate)
  CMakeLists.txt:15 (find_package)


-- Configuring incomplete, errors occurred!
FATAL ERROR: command exited with status 1: /usr/local/bin/cmake -DWEST_PYTHON=/usr/bin/python3 -B/app/build/left -S/app/zmk/app -GNinja -DBOARD=adv360_left -DZMK_CONFIG=/app/config
make: *** [Makefile:15: firmware/$(TIMESTAMP)-left.uf2] Error 1

though it works if I replace $(PWD) with ./ there...

diff --git a/Makefile b/Makefile
index 35c0006..2f89664 100644
--- a/Makefile
+++ b/Makefile
@@ -11,9 +11,10 @@ clean:
        rm -f firmware/*.uf2

 firmware/%-left.uf2 firmware/%-right.uf2: config/adv360.keymap
-       $(DOCKER) run --rm -it --name zmk \
-               -v $(PWD)/firmware:/app/firmware \
-               -v $(PWD)/config:/app/config:ro \
+       echo $(PWD)
+       $(DOCKER)  run --rm -it --name zmk \
+               -v ./firmware:/app/firmware \
+               -v ./config:/app/config:ro \
                -e TIMESTAMP=$(TIMESTAMP) \
                zmk

honestly I'd probably just keep $(PWD) in the firmware target, it's less ambiguous than ./

Unrelated...

I think the rest of this feedback is at risk of causing feature creep for a patch that was simply meant to cause make to not fail when rm-ing non-existant files 😄

The file(s) being removed may not exist, but make shouldn't fail because
of it:

        $ make clean
        rm ./firmware/*.uf2
        rm: cannot remove './firmware/*.uf2': No such file or directory
        make: *** [Makefile:10: clean] Error 1
        $ echo $?
        2

this also removes leading ./ from path in clean, and assumes that `make
clean` is being run from the top level repo dir
@ghost
Copy link

ghost commented Oct 24, 2022

Just rename the pull request then. We are talking about a few lines in the Makefile. Or if you don't want to do it, say so, and I will submit a PR for it (I would just like it done).

@craftyguy
Copy link
Contributor Author

craftyguy commented Oct 25, 2022 via email

@ReFil
Copy link
Collaborator

ReFil commented Oct 25, 2022

I'd definitely prefer the other changes being proposed to be in a separate PR

@ghost
Copy link

ghost commented Oct 25, 2022

So let's get those changes in and I will file another PR when this gets merged.

@ReFil ReFil merged commit 754f133 into KinesisCorporation:V2.0 Oct 30, 2022
stefanradziuk pushed a commit to stefanradziuk/adv360-zmk that referenced this pull request Nov 13, 2022
…an-no-fail

makefile: ignore errors during clean
stefanradziuk pushed a commit to stefanradziuk/adv360-zmk that referenced this pull request Nov 13, 2022
…an-no-fail

makefile: ignore errors during clean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants