Skip to content

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

Merged
Enoch247 merged 1 commit intoRIOT-OS:masterfrom
bmewen:cachedir-tag
Jun 18, 2024
Merged

Makefile.include: creation of CACHEDIR.TAG as a dependency of pkg-prepare#20689
Enoch247 merged 1 commit intoRIOT-OS:masterfrom
bmewen:cachedir-tag

Conversation

@bmewen
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

bmewen commented May 23, 2024 •
edited
Loading

@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

kfessel
kfessel reviewed May 23, 2024
View reviewed changes
Enoch247
Enoch247 reviewed May 24, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@bmewen bmewen requested review from Enoch247 and kfessel May 24, 2024 07:39
kfessel
kfessel reviewed May 24, 2024
View reviewed changes
@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
Copy Markdown
Contributor Author

bmewen commented Jun 6, 2024

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

@kfessel
Copy link
Copy Markdown
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
Enoch247
Enoch247 requested changes Jun 7, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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.

kfessel
kfessel approved these changes Jun 7, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown

riot-ci commented Jun 7, 2024 •
edited
Loading

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
Copy Markdown
Contributor

Enoch247 commented Jun 8, 2024

Thanks, I will test and review this weekend.

Enoch247
Enoch247 requested changes Jun 10, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@github-actions github-actions bot added the Area: pkg Area: External package ports label Jun 10, 2024
@bmewen bmewen requested a review from Enoch247 June 10, 2024 08:20
@bmewen
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Enoch247 commented Jun 18, 2024

Thank you. I will take a look tomorrow.

Enoch247
Enoch247 approved these changes Jun 18, 2024
View reviewed changes
pkg/pkg.mk
# Declare 'all' first to have it being the default target
all: prepare

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

@Enoch247 Enoch247 Jun 18, 2024

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
@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

Reviewers

@kfessel kfessel kfessel approved these changes

@Enoch247 Enoch247 Enoch247 approved these changes

Assignees

@Enoch247 Enoch247

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bmewen @kfessel @riot-ci @Enoch247 @mguetschow