Skip to content

Makefile.include: discover object files in sub-paths while linking#15364

Merged
cgundogan merged 1 commit intoRIOT-OS:masterfrom
leandrolanzieri:pr/makefiles/discover_object_files_subpaths
Nov 4, 2020
Merged

Makefile.include: discover object files in sub-paths while linking#15364
cgundogan merged 1 commit intoRIOT-OS:masterfrom
leandrolanzieri:pr/makefiles/discover_object_files_subpaths

Conversation

@leandrolanzieri
Copy link
Contributor

@leandrolanzieri leandrolanzieri commented Nov 3, 2020

Contribution description

The find command that collects the object files while linking is currently too strict and does not include sub-directories within the module's directories. This causes use cases like the described in #15357 to fail. This allows a recursive search for object files in those directories.

I've tested this following the steps in #15357 and now it works properly.

Testing procedure

Issues/PRs references

Fixes #15357
Issue introduced in #14754

@leandrolanzieri leandrolanzieri added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 labels Nov 3, 2020
@leandrolanzieri leandrolanzieri mentioned this pull request Nov 3, 2020
Closed
larseggert
larseggert approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@larseggert larseggert left a comment

Choose a reason for hiding this comment

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

Fixes the issue

cgundogan
cgundogan approved these changes Nov 3, 2020
View reviewed changes
Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

Change is sensible. Tested as outlined in the PR description and works for me. @larseggert does the proposed change also work for you? (maybe also for other test cases, if there are any?)

@cgundogan
Copy link
Member

cgundogan commented Nov 3, 2020

Fixes the issue

I was too slow (:

@cgundogan cgundogan added 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 labels Nov 3, 2020
@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 3, 2020
@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 3, 2020
cgundogan
cgundogan requested changes Nov 3, 2020
View reviewed changes
Copy link
Member

@cgundogan cgundogan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

find always returns another order of object files for linking. This yields a random offset for symbols in the elf file. By sorting the output of find, the elf file generation should be deterministic and the hash compare test should succeed.

@cgundogan
Copy link
Member

cgundogan commented Nov 3, 2020

you can squash the suggestions directly

@leandrolanzieri leandrolanzieri force-pushed the pr/makefiles/discover_object_files_subpaths branch from 1dc479b to f229276 Compare November 3, 2020 16:25
cgundogan
cgundogan approved these changes Nov 4, 2020
View reviewed changes
Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

ACK. Murdock is also green @bergzand should this be backported?

@cgundogan cgundogan merged commit 58c0b47 into RIOT-OS:master Nov 4, 2020
@leandrolanzieri leandrolanzieri deleted the pr/makefiles/discover_object_files_subpaths branch November 4, 2020 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgundogan cgundogan cgundogan approved these changes

@miri64 miri64 Awaiting requested review from miri64

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

+1 more reviewer

@larseggert larseggert larseggert approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Link failures since 81cb769

3 participants

@leandrolanzieri @cgundogan @larseggert