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.include: creation of CACHEDIR.TAG as a dependency of pkg-prepare #20689

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

bmewen
Copy link
Contributor

@bmewen bmewen commented May 23, 2024

Contribution description

The cached dir, only build for now, are not created by default so I added a small default target that will create the build dir and the CACHEDIR whenever a make command is called.

Testing procedure

Both commands should have the same behavior now. Previously pkg-prepare was not creating build/CACHEDIR.TAG and was creating the build/ directory through pkg/pkg.mk.

make pkg-prepare
make

Issues/PRs references

N/A

@github-actions github-actions bot added the Area: build system Area: Build system label May 23, 2024
@bmewen
Copy link
Contributor Author

bmewen commented May 23, 2024

@Enoch247 you can look at the way I've done it, not sure it's the best way but at least I think it's the intended behavior

Makefile.include Outdated Show resolved Hide resolved
Copy link
Contributor

@Enoch247 Enoch247 left a comment

Choose a reason for hiding this comment

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

You might consider taking a look at order only prerequisites to accomplish your goal.

You could leave the existing rule to create the tag file as-is. Then just add it as an order-only-prereq to any rules that should create the build dir.

Makefile.include Outdated Show resolved Hide resolved
Makefile.include Outdated Show resolved Hide resolved
Makefile.include Outdated Show resolved Hide resolved
@bmewen bmewen requested a review from kfessel May 27, 2024 12:25
@bmewen bmewen marked this pull request as ready for review May 28, 2024 10:53
@bmewen
Copy link
Contributor Author

bmewen commented Jun 6, 2024

I am still waiting for a review whenever someone is available. Thank you

@kfessel
Copy link
Contributor

kfessel commented Jun 6, 2024

I am still waiting for a review whenever someone is available. Thank you

From my POV this PR is a strange workaround abusing include for a minor problem (not many people call pkg-prepare directly)

  • the include will search for the file at more than one location and include it if found https://www.gnu.org/software/make/manual/make.html#Include First, any directories you have specified with the ‘-I’ or ‘--include-dir’ options are searched (see [Summary of Options](https://www.gnu.org/software/make/manual/make.html#Options-Summary)). Then the following directories (if they exist) are searched, in this order: prefix/include (normally /usr/local/include [1](https://www.gnu.org/software/make/manual/make.html#FOOT1)) /usr/gnu/include, /usr/local/include, /usr/include.

  • you identified one rule where the file should be generated but is not - there is a more straight forward fix for that ( just add a requirement to that rule

  • searching for mkdir -p or ${BUILD_DIR} did not reveal an unmanageable number of instances that might need fixing in future

So in this form i don't think it is good to go (no positive from me)
but i also write this review as a comment -> easy to ignore (no negative)

@Enoch247 Enoch247 self-assigned this Jun 7, 2024
Copy link
Contributor

@Enoch247 Enoch247 left a comment

Choose a reason for hiding this comment

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

The way you have used the include here is really clever, but not the right tool for the job in my opinion. The problem with doing it this way is that it is always run for every possible invocation of make even invocations that should not create anything such as any of the info-* recipes. This could be a bit of a surprise in environments when write access is not expected. Also, this causes the build dir to be create always, even if not needed (for example make BOARD=native examples/blinky).

@bmewen bmewen changed the title Makefile.include: create cached dir and CACHEDIR.TAG by default (only build for now) Makefile.include: creation of CACHEDIR.TAG as a dependency of pkg-prepare Jun 7, 2024
@bmewen
Copy link
Contributor Author

bmewen commented Jun 7, 2024

I removed what I have done previously to move the creation of CACHEDIR.TAG as a dependency of pkg-prepare, it fixes my issue and is anyway enough for me. I appreciate the review thanks.

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution

@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 7, 2024
@riot-ci
Copy link

riot-ci commented Jun 7, 2024

Murdock results

✔️ PASSED

b275e61 Makefile.include: creation of CACHEDIR.TAG as a dependency of pkg-prepare

Success Failures Total Runtime
10161 0 10161 24m:38s

Artifacts

@Enoch247
Copy link
Contributor

Enoch247 commented Jun 8, 2024

Thanks, I will test and review this weekend.

Copy link
Contributor

@Enoch247 Enoch247 left a comment

Choose a reason for hiding this comment

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

I would prefer to see the prepare target inside of pkg.mk depend on $(BUILD_DIR)/CACHEDIR.TAG, or the prepare target depending on '$(PKGDIRBASE)' and '$(PKGDIRBASE)' on $(BUILD_DIR)/CACHEDIR.TAG, but I won't insist on it.

If you decide to keep the current approach, please accept the suggested change for line 807.

Makefile.include Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Area: pkg Area: External package ports label Jun 10, 2024
@bmewen
Copy link
Contributor Author

bmewen commented Jun 17, 2024

@Enoch247 this version is good for me if you have time to review it once more.

@Enoch247
Copy link
Contributor

Thank you. I will take a look tomorrow.

@@ -82,8 +82,15 @@ PKG_CUSTOM_PREPARED ?=
# Declare 'all' first to have it being the default target
all: prepare

BUILD_DIR ?= $(RIOTBASE)/build
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this recipe should be placed outside of this file, as it is at a broader scope than building packages. Perhaps as an include-able makefile in ./makefiles. However, I will approve as-is.

@Enoch247 Enoch247 added this pull request to the merge queue Jun 18, 2024
Merged via the queue into RIOT-OS:master with commit aee2ea6 Jun 18, 2024
26 checks passed
@mguetschow mguetschow added this to the Release 2024.07 milestone Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants