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

remove version comment fixing build issue #18489

Merged
merged 1 commit into from
Aug 21, 2022
Merged

remove version comment fixing build issue #18489

merged 1 commit into from
Aug 21, 2022

Conversation

nica-f
Copy link

@nica-f nica-f commented Aug 21, 2022

Contribution description

This change resolves a built issue cause by the added comment to the release tag in the Makefile.

Testing procedure

Tested by compiling a project using LVGL.

Issues/PRs references

N/A

@github-actions github-actions bot added the Area: pkg Area: External package ports label Aug 21, 2022
@chrysn
Copy link
Member

chrysn commented Aug 21, 2022

To test, I think you have to have an old checkout of the pkg; trying...

@chrysn
Copy link
Member

chrysn commented Aug 21, 2022

Testing is highly nontrivial because it only triggers if the relevant hash is not present in the existing checkout -- merely cleaning, going to 2022.04, running the test, going to master and running the test again does not trigger the failure, because if you do that now, then you get a recent checkout and the requested hash is already present.

So, steps to test are (not necessarily minimized):

$ make -C tests/pkg_lvgl
$ cd build/pkg
$ rm -rf lvg
$ git clone https://github.com/littlevgl/lvgl -b fix/img-transform-precision --depth=1
$ cd -
$ make -C tests/pkg_lvgl

Then you get:

fatal: invalid refspec '9024b72b4853e1e7ac29a42e54b7a10d3c4f3b20  '
make[1]: *** [/home/chrysn/git/RIOT/pkg/pkg.mk:125: /home/chrysn/git/RIOT/build/pkg/lvgl/.pkg-state.git-downloaded] Error 128

To quote @nica-f: "Ein schöner corner corner corner case".

The presented patch fixes this.

I suggest that we remove all these comments tree-wide, as that's a very subtle thing to run into.

@chrysn chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Area: build system Area: Build system labels Aug 21, 2022
@chrysn chrysn enabled auto-merge August 21, 2022 12:16
@chrysn chrysn merged commit 777e148 into RIOT-OS:master Aug 21, 2022
@kaspar030
Copy link
Contributor

I suggest that we remove all these comments tree-wide, as that's a very subtle thing to run into.

... or move them to another line, they do provide information.

The reasons why we use hashes in the first place are, 1.) git-cache is flakey with tags, and 2.) only hashes ensure immutability.
But IMO 1.) might get fixed and for git tags, we might want to ignore 2.) for friendlyness towards human developers, once git-cache is tested to work poperly with tags. At that point, I'd rather not have to look for the tag names.

chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request Aug 21, 2022
Makefiles don't do comments, so these were forwarded into the variable.
*Most* users would expand the arguments to a shell where it'd be
ignored, but not all of them.

Contributes-To: RIOT-OS#18489

(This is also where the one version that is added here was removed).
chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request Sep 26, 2022
Makefiles don't do comments, so these were forwarded into the variable.
*Most* users would expand the arguments to a shell where it'd be
ignored, but not all of them.

Contributes-To: RIOT-OS#18489

(This is also where the one version that is added here was removed).
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
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 CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants