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

pkg/pkg.mk: make sure we are building the correct commit. #11129

Merged
merged 1 commit into from Mar 19, 2019

Conversation

jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Mar 7, 2019

Contribution description

This solves an issue I ran into while trying to figure out what was happening with edbg. I had two git worktrees and I had two different reset behaviours even though the PKG_VERSION of edbg was the same in both trees. Upon closer inspection I discovered that in one tree PKG_VERSION and the actual HEAD commit of edbg did not match.

Once the repo is downloaded, the version is not checked gain. This means that even if a package's Makefile is touched, that will only cause a rebuild of the previously downloaded version. For packages
that are normally not deleted, like edbg, this renders PKG_VERSION bumps ineffective, unless the user manually deletes the repo directory.

This patch fixes that by always checking that the repo is at the right commit.

Testing procedure

I included a throwaway commit to make testing easier.

After checking out this, the first step is to go backwards to the commits before this PR, clean edbg, and download it and build it again:

# go before my first commit
$ git checkout 6ff1a57^
$ rm -rf dist/tools/edbg/edbg dist/tools/edbg/bin
$ make -C examples/hello-world $(pwd)/dist/tools/edbg/edbg
$ grep VERSION dist/tools/edbg/Makefile && git -C dist/tools/edbg/bin rev-parse HEAD
PKG_VERSION=807d948cc8a664ade3e9446b4937aa54268afcb2
807d948cc8a664ade3e9446b4937aa54268afcb2

Now let's try to bump the version:

$ git checkout 6ff1a57
$ make -C examples/hello-world $(pwd)/dist/tools/edbg/edbg
$ grep VERSION dist/tools/edbg/Makefile && git -C dist/tools/edbg/bin rev-parse HEAD
PKG_VERSION=ba864ebc46e985cb400ccdd63cdafaccbc3698c0
807d948cc8a664ade3e9446b4937aa54268afcb2

D'oh! that's not what we want. Now with the fix:

$ git checkout a0c709f
$ touch dist/tools/edbg/Makefile
$ make -C examples/hello-world $(pwd)/dist/tools/edbg/edbg
$ grep VERSION dist/tools/edbg/Makefile && git -C dist/tools/edbg/bin rev-parse HEAD
PKG_VERSION=ba864ebc46e985cb400ccdd63cdafaccbc3698c0
ba864ebc46e985cb400ccdd63cdafaccbc3698c0

Issues/PRs references

Found when investigating #11125 .

@jcarrano jcarrano added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system labels Mar 7, 2019
@@ -1,6 +1,6 @@
PKG_NAME=edbg
PKG_URL=https://github.com/ataradov/edbg
PKG_VERSION=807d948cc8a664ade3e9446b4937aa54268afcb2
PKG_VERSION=ba864ebc46e985cb400ccdd63cdafaccbc3698c0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: this is for testing only.

Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Interesting feature. Works like a charm with SAML21-XPRO !
Not a makefile expert so I leave the in-depth review to someone else but this PR should be not be harmful.

@dylad dylad added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 13, 2019
@jcarrano jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 13, 2019
@dylad
Copy link
Member

dylad commented Mar 14, 2019

one side note and I don't know if it's feasible or not.
Would it be useful to add a visible warning (yellow line ?) to inform user that edbg cannot be rebuild due to lack of internet connection so it keeps the previous version ? Just wondering.

@jcarrano
Copy link
Contributor Author

@dylad I think that we should see versions (at least now, with the current model) as a "hard" requirement. The reason is that it is impossible to know if the old version works fine, or has incompatibilities, or a security issue or a bug. If we were using a kind of "version number" scheme, we would be able to be a bit smarter on wether we can fulfill the requirements without upgrading, but as it is now, allowing that kind of behavior can possibly lead to errors.

Why not define an "update-tools" target instead? That way you can be sure that you have up-to date tools for whatever version of RIOT you may be using.

@dylad
Copy link
Member

dylad commented Mar 17, 2019

ACK on my side.

@jcarrano could you squash please ?

Why not define an "update-tools" target instead? That way you can be sure that you have up-to date tools for whatever version of RIOT you may be using.

This is an interesting idea, but I'm quite happy with this PR as it.

@dylad dylad added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Mar 17, 2019
@dylad dylad self-assigned this Mar 17, 2019
@jcarrano
Copy link
Contributor Author

I removed the test commit.

Why not define an "update-tools" target instead? That way you can be sure that you have up-to date tools for whatever version of RIOT you may be using.

A problem with my suggestion is that it would require having a central listing of all tools and manually maintaining it, which is ugly.

Once the repo is downloaded, the version is not checked gain. This
means that even if a package's Makefile is touched, that will only
cause a rebuild of the previously downloaded version. For packages
that are normally not deleted, like edbg, this renders PKG_VERSION
bumps ineffective, unless the user manually deletes the repo
directory.

This patch fixes that by always checking that the repo is at the
right commit.
@jcarrano jcarrano added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 19, 2019
@jcarrano jcarrano merged commit 5488c5f into RIOT-OS:master Mar 19, 2019
@danpetry danpetry added this to the Release 2019.04 milestone Mar 26, 2019
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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants