Makefile.base: fix AR keeping removed source files objects by cladmi · Pull Request #7952 · RIOT-OS/RIOT

cladmi · 2017-11-07T17:42:00Z

AR incrementally adds file without removing files.
If a c file is deleted or disabled(submodule removal) it is not removed from
archive and still ends up in the final elf file.

This fix removes the need to do 'make clean' for this case.

However it will break cases where an APPLICATION and a MODULE or two modules
have the same name and only worked because source files names where different.

This requires: ~~#7951~~ to not break compilation. I only tested native and samr21-xpro right now.

Edit: This also requires ~~#8019~~, I added a fix that will be proposed as a separate PR. The only important file change for this PR is Makefile.base.

cladmi · 2017-11-10T11:11:41Z

I enable murdock to show the broken output and see if other fixes than #7951 could be necessary.

cladmi · 2017-11-13T14:51:25Z

It also requires fix for mips cpus to work #8019
I will do a proper PR for it.

kYc0o · 2018-02-28T13:30:59Z

Is this still waiting for other PR?

cladmi · 2018-03-01T11:21:44Z

It should not be anymore.

I will add another PR to add a note in the http://doc.riot-os.org/creating-modules.html page to mention that MODULE names should be unique because with this it will be explicitly enforced.

kYc0o · 2018-03-01T11:24:55Z

I will add another PR to add a note in the http://doc.riot-os.org/creating-modules.html page to mention that MODULE names should be unique because with this it will be explicitly enforced.

I'd say put it here, it's the right context and more easy to find if everything is here.

cladmi · 2018-03-01T13:09:29Z

I added some lines in the section talking about the MODULE name.

kYc0o · 2018-03-01T15:57:30Z

doc/doxygen/src/creating-modules.md

+### Pitfalls ###
+
+The `MODULE` name should be uniq or build will break as modules overwrite
+the same output file.


I suggest to change this to "unique" or build breaks.

kYc0o · 2018-03-01T15:58:20Z

doc/doxygen/src/creating-modules.md

+ * Packages root directory (libfixmath/u8g2)
+ * boards/cpu/periph and their common boards/cpu/periph
+
+Note: even if all boards and cpus implement the `board` and `cpu` modules only


I suggest to rephrase: "... modules, only ..."

kYc0o

Looks ok but I have some doubts...

kYc0o · 2018-03-01T15:59:07Z

Makefile.base

-	$(Q)$(AR) $(ARFLAGS) $@ $?
+	@# Recreate archive to cleanup deleted/non selected source files objects
+	$(Q)$(RM) $@
+	$(Q)$(AR) $(ARFLAGS) $@ $^


Are we sure we want to remove it ALWAYS, and not only under certain conditions?

Yes always, because we want to re-create a new archive. If not, old files are kept as ar is incremental.

When doing gcc -o out.o it overwrites the previous object with the new one, and that is what we expect.
The RM here, is implementing an --overwrite option that ar does not have.

cladmi · 2018-03-05T14:21:56Z

Updated the doc as you suggested.

cladmi · 2018-03-06T12:39:50Z

I re-worded the commit messages to fix "unique" too.

cladmi · 2018-03-14T11:15:39Z

@kYc0o Do you like the changes ? And so can I rebase.

kYc0o · 2018-03-14T11:24:40Z

Yep, let's go ahead.

kYc0o · 2018-03-14T11:25:29Z

Murdock, complains though. Can you check?

cladmi · 2018-03-14T11:28:23Z

I think it's on the fixup, I will rebase to re-run.

AR incrementally adds file without removing files. If a c file is deleted or disabled(submodule removal) it is not removed from archive and still ends up in the final elf file. This fix removes the need to do 'make clean' for this case. However it will break cases where an APPLICATION and a MODULE or two modules have the same name and only worked because source files names where different.

Modules produce an output archive called `${MODULE}.a` if several modules use the same name, the output is overwritten.

cladmi mentioned this pull request Nov 7, 2017

examples/pkgs: rename modules with duplicate names #7951

Merged

cladmi added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels Nov 7, 2017

miri64 assigned kaspar030 Nov 7, 2017

kaspar030 added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 9, 2017

cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 10, 2017

cladmi force-pushed the pr/ar_delete_old_files branch 3 times, most recently from e830f10 to 688a90c Compare November 13, 2017 11:44

cladmi mentioned this pull request Nov 13, 2017

Module 'cpu' name conflict for mips_pic32mx and mips_pic32mz #8019

Closed

cladmi force-pushed the pr/ar_delete_old_files branch from 688a90c to 5fea2ae Compare November 13, 2017 14:31

cladmi force-pushed the pr/ar_delete_old_files branch 2 times, most recently from 80057d9 to 3a71827 Compare November 14, 2017 12:19

cladmi force-pushed the pr/ar_delete_old_files branch from 3a71827 to e56ca23 Compare January 26, 2018 18:04

cladmi removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 26, 2018

cladmi force-pushed the pr/ar_delete_old_files branch from e56ca23 to cf1f032 Compare January 29, 2018 14:29

cladmi added the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 29, 2018

cladmi mentioned this pull request Jan 29, 2018

cpu/efm32: fix 'cpu' module name conflict #8471

Merged

cladmi force-pushed the pr/ar_delete_old_files branch 2 times, most recently from f3e4e8e to c729d32 Compare January 30, 2018 13:37

cladmi mentioned this pull request Jan 30, 2018

makefiles: Add an APPLICATION_MODULE variable #8492

Merged

kYc0o 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 Feb 28, 2018

cladmi force-pushed the pr/ar_delete_old_files branch from c729d32 to 710f4d2 Compare March 1, 2018 10:55

kYc0o reviewed Mar 1, 2018

View reviewed changes

kYc0o requested changes Mar 1, 2018

View reviewed changes

cladmi force-pushed the pr/ar_delete_old_files branch from a1958fb to 03ea7ba Compare March 6, 2018 12:38

cladmi requested a review from jnohlgard March 9, 2018 10:31

kYc0o approved these changes Mar 14, 2018

View reviewed changes

cladmi added 2 commits March 14, 2018 12:29

doc: add warning that MODULE name should be unique

e5d234e

Modules produce an output archive called `${MODULE}.a` if several modules use the same name, the output is overwritten.

cladmi force-pushed the pr/ar_delete_old_files branch from 03ea7ba to e5d234e Compare March 14, 2018 11:29

kYc0o merged commit 3af570b into RIOT-OS:master Mar 14, 2018

cladmi deleted the pr/ar_delete_old_files branch March 15, 2018 12:56

cladmi added this to the Release 2018.04 milestone Nov 7, 2018

Conversation

cladmi commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Nov 10, 2017

Uh oh!

cladmi commented Nov 13, 2017

Uh oh!

kYc0o commented Feb 28, 2018

Uh oh!

cladmi commented Mar 1, 2018

Uh oh!

kYc0o commented Mar 1, 2018

Uh oh!

cladmi commented Mar 1, 2018

Uh oh!

kYc0o Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

kYc0o Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

kYc0o Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Mar 6, 2018

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

kYc0o commented Mar 14, 2018

Uh oh!

kYc0o commented Mar 14, 2018

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cladmi commented Nov 7, 2017 •

edited

Loading

cladmi commented Mar 5, 2018 •

edited

Loading