Bug 333308 (makeclean)

make clean and make distclean miss various files

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: jag (Peter Annema), Assigned: jag (Peter Annema))

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 7 obsolete attachments)

1.11 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
480 bytes, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
546 bytes, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
12.91 KB, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
1.60 KB, patch
cls
: review+
Benjamin Smedberg
: approval-branch-1.8.1+
Details | Diff | Splinter Review
617 bytes, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
153.40 KB, text/plain
Details
13.98 KB, patch
jag (Peter Annema)
: superreview+
Mike Schroepfer
: approval1.9+
Details | Diff | Splinter Review
3.62 KB, patch
Benjamin Smedberg
: review+
Mike Schroepfer
: approval1.9+
Details | Diff | Splinter Review
1.16 KB, patch
Benjamin Smedberg
: review+
damons
: approval1.9+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
To reproduce, just do a build of your favourite product and do a make distclean and see what's left behind (easier if you do this in a separate OBJDIR).
(Assignee)

Comment 1

12 years ago
Created attachment 217729 [details] [diff] [review]
Remove generated / copied files
Attachment #217729 - Flags: superreview?(neil)
Attachment #217729 - Flags: review?(neil)
(Assignee)

Comment 2

12 years ago
Created attachment 217733 [details] [diff] [review]
This is redundant

Initially I was gonna add SDK_XPIDLSRCS to that check, but I found that XPIDL_GEN_DIR is already appended to GARBAGE_DIRS for both those cases around line 1407
Attachment #217733 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #217733 - Flags: review? → review?(benjamin)

Updated

12 years ago
Attachment #217729 - Flags: superreview?(neil)
Attachment #217729 - Flags: superreview+
Attachment #217729 - Flags: review?(neil)
Attachment #217729 - Flags: review+
(Assignee)

Comment 3

12 years ago
Created attachment 217738 [details] [diff] [review]
Clean up _img_list
Attachment #217738 - Flags: review?(benjamin)

Updated

12 years ago
Attachment #217733 - Flags: review?(benjamin) → review+

Updated

12 years ago
Attachment #217738 - Flags: review?(benjamin) → review+
Did this patch turn "WINNT 5.2 gaius Dep" orange? ("Trender:[FAILED]")

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox
(Assignee)

Comment 5

12 years ago
Nope.
(Assignee)

Comment 6

12 years ago
Here's a list of files that are currently left behind after make distclean in a SeaMonkey debug OBJDIR:

./gfx/gfx-config.h
./xpcom/xpcom-private.h
./xpcom/xpcom-config.h
./netwerk/necko-config.h
./build/unix/seamonkey-config

./extensions/xmlextras/base/src/nsRect.cpp

./extensions/p3p/p3p200010.xsl
./extensions/p3p/p3p200005.xsl
./extensions/p3p/p3p200201.xsl
./extensions/p3p/p3p200109.xsl
./extensions/p3p/p3p200012.xsl

./directory/c-sdk/config/autoconf.mk
./directory/c-sdk/ldap/libraries/libutil/Makefile
./directory/c-sdk/ldap/libraries/libssldap/Makefile
./directory/c-sdk/ldap/clients/tools/Makefile

./nsprpub/lib/tests/Makefile
./nsprpub/pkg/solaris/SUNWpr/Makefile
./nsprpub/pkg/solaris/Makefile
./nsprpub/pkg/solaris/SUNWprd/Makefile
./nsprpub/pkg/Makefile
./nsprpub/pkg/linux/Makefile

./security/manager/.nss.cleaned
The following files are left over after a make distclean on xulrunner 1.8:
build/unix/xulrunner-config
extensions/java/xpcom/interfaces/.done
extensions/java/xpcom/src/.done
extensions/pref/system-pref/src/gconf/nsSystemPrefFactory.cpp
extensions/xmlextras/base/src/nsRect.cpp
gfx/gfx-config.h
modules/libpr0n/build/_img_list
netwerk/necko-config.h
nsprpub/lib/tests/Makefile
nsprpub/pkg/linux/Makefile
nsprpub/pkg/solaris/SUNWpr/Makefile
nsprpub/pkg/solaris/SUNWprd/Makefile
nsprpub/pkg/solaris/Makefile
nsprpub/pkg/Makefile
profile/dirserviceprovider/standalone/nsProfileDirServiceProvider.cpp
profile/dirserviceprovider/standalone/nsProfileLock.cpp
security/manager/.nss.cleaned
toolkit/library/dlldeps-javaxpcom.cpp
toolkit/library/dlldeps-obs.cpp
toolkit/library/dlldeps.cpp
toolkit/mozapps/downloads/src/nsHelperAppDlg.js
toolkit/mozapps/extensions/src/nsExtensionManager.js
toolkit/mozapps/update/src/nsUpdateService.js
toolkit/xre/nsSigHandlers.cpp
toolkit/xre/nsStackFrameUnix.cpp
toolkit/xre/nsStackFrameUnix.h
toolkit/xre/showOSAlert.cpp
xpcom/build/nsCOMPtr.cpp
xpcom/build/nsComponentManagerUtils.cpp
xpcom/build/nsDebug.cpp
xpcom/build/nsGREGlue.cpp
xpcom/build/nsGenericFactory.cpp
xpcom/build/nsID.cpp
xpcom/build/nsIInterfaceRequestorUtils.cpp
xpcom/build/nsINIParser.cpp
xpcom/build/nsMemory.cpp
xpcom/build/nsTHashtable.cpp
xpcom/build/nsTraceRefcnt.cpp
xpcom/build/nsVersionComparator.cpp
xpcom/build/nsWeakReference.cpp
xpcom/glue/standalone/nsCOMPtr.cpp
xpcom/glue/standalone/nsComponentManagerUtils.cpp
xpcom/glue/standalone/nsDebug.cpp
xpcom/glue/standalone/nsGREGlue.cpp
xpcom/glue/standalone/nsID.cpp
xpcom/glue/standalone/nsIInterfaceRequestorUtils.cpp
xpcom/glue/standalone/nsINIParser.cpp
xpcom/glue/standalone/nsMemory.cpp
xpcom/glue/standalone/nsTHashtable.cpp
xpcom/glue/standalone/nsTraceRefcnt.cpp
xpcom/glue/standalone/nsVersionComparator.cpp
xpcom/glue/standalone/nsWeakReference.cpp
xpcom/xpcom-config.h
xpcom/xpcom-private.h
xpfe/components/filepicker/src/nsFilePicker.js

The following file is *removed* by the distclean, while it is in the source tarball.
security/coreconf/nsinstall/nfspwd

I'm going to attach a patch for all these.
Created attachment 218131 [details] [diff] [review]
Remove more

I still need to check what still needs to be applied after the patches already attached here, and against the trunk. The nfspwd fix is not required any more, for instance. It has been fixed in bug #311074.

There are some changes that could be done otherwise:
- The leftover .js files could be installed as EXTRA_PP_COMPONENTS instead of EXTRA_COMPONENTS, but that would mean renaming the .js.in files to .js.
- The _img_list seems to be useless, it could be plain removed (i.e. not generated at all)
- Maybe nsprpub/pkg could be removed, but I'm not sure if it's any useful for anyone.
Attachment #218131 - Flags: review?(benjamin)

Comment 9

12 years ago
Comment on attachment 218131 [details] [diff] [review]
Remove more

>=== nsprpub/Makefile.in

NSPR patches need to be attached separately and reviewed by wtchang.

>=== security/coreconf/nsinstall/Makefile

NSS patches need to be attached separately and reviewed by nelsonb or wtchang. But I'm pretty sure this was already done on trunk, and shouldn't be in this patch.

r=me without those files. Could you please provide an updated patch which doesn't have those files, and which will apply to trunk?
Attachment #218131 - Flags: review?(benjamin) → review+
> >=== security/coreconf/nsinstall/Makefile
> 
> NSS patches need to be attached separately and reviewed by nelsonb or wtchang.
> But I'm pretty sure this was already done on trunk, and shouldn't be in this
> patch.

This is indeed the case
 
> r=me without those files. Could you please provide an updated patch which
> doesn't have those files, and which will apply to trunk?

Patches coming.
Created attachment 218726 [details] [diff] [review]
Remove more for trunk ; nsprpub excluded

New version against trunk, without nsprpub files
Attachment #218131 - Attachment is obsolete: true
Attachment #218726 - Flags: review?(benjamin)
Created attachment 218727 [details] [diff] [review]
Remove more for trunk ; nsprpub part (checked in on moz & nspr trunks)
Attachment #218727 - Flags: review?(wtchang)

Updated

12 years ago
Attachment #218727 - Flags: review?(wtchang) → review+

Updated

12 years ago
Attachment #218727 - Attachment description: Remove more for trunk ; nsprpub part → Remove more for trunk ; nsprpub part (checked in on moz & nspr trunks)
Attachment #218727 - Flags: approval-branch-1.8.1?

Comment 13

12 years ago
Comment on attachment 218727 [details] [diff] [review]
Remove more for trunk ; nsprpub part (checked in on moz & nspr trunks)

cls, if you meant to ask me for approval-branch1.8.1, a=me... or maybe you meant to ask wtchang?
Attachment #218727 - Flags: approval-branch-1.8.1? → approval-branch-1.8.1+

Comment 14

12 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #218726 - Flags: review?(benjamin) → review+
Created attachment 224231 [details] [diff] [review]
Some more cleaning

Some leftovers in firefox build
Attachment #224231 - Flags: review?(benjamin)

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Attachment #224231 - Flags: review?(benjamin) → review+

Comment 16

11 years ago
Landed attachment 224231 [details] [diff] [review] on trunk.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
Created attachment 229965 [details] [diff] [review]
Even more cleanup

Note the allmakefiles.sh part applies to the 1.8 branch only. The xmlextras/base reference don't exist on trunk, and the files exist in 1.8.0 branch, so they need to be created. They got removed in 1.8 branch, though, as it seems.

The rest is against firefox 2.0b1, it may apply to trunk as well.
Attachment #229965 - Flags: review?(benjamin)
Reopening for the new patch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 19

11 years ago
Comment on attachment 229965 [details] [diff] [review]
Even more cleanup

>=== config/Makefile.in

>+GARBAGE += system_wrappers

You mean GARBAGE_DIRS, right?
Attachment #229965 - Flags: review?(benjamin) → review-
Created attachment 230605 [details] [diff] [review]
Even more cleanup... fixed
Attachment #229965 - Attachment is obsolete: true
Attachment #230605 - Flags: review?(benjamin)

Updated

11 years ago
Attachment #230605 - Flags: review?(benjamin) → review+
1.
The last patch (attachment 230605 [details] [diff] [review]) haven't been checked in,
though the last comment (comment #20) is a bit confusing.

2.
bug 336244 removed extensions/xmlextras/base so there's
no need to delete those files, on trunk

3.
nsMicrosummaryService.js.in was renamed to nsMicrosummaryService.js,
(see bug 354449) so there's no need to delete it on trunk.

i.e. now the useful part of the patch is only

>=== config/Makefile.in
>==================================================================
>--- config/Makefile.in	(revision 13)
>+++ config/Makefile.in	(local)
>@@ -136,6 +136,8 @@
> 	if test ! -d system_wrappers; then mkdir system_wrappers; fi
> 	$(PERL) $(topsrcdir)/nsprpub/config/make-system-wrappers.pl >system_wrappers < $(srcdir)/system-headers
> 	$(INSTALL) system_wrappers $(DIST)/include
>+
>+GARBAGE_DIRS += system_wrappers
> endif
> 
> # we don't use an explicit dependency here because then we would
Created attachment 254064 [details] [diff] [review]
Yet some more for suite
Attachment #254064 - Flags: review?(ajvincent)
Comment on attachment 254064 [details] [diff] [review]
Yet some more for suite

I'm not a reviewer for makefile changes; per biesi's advice, requesting review from mentovai.
Attachment #254064 - Flags: review?(ajvincent) → review?(mark)

Updated

11 years ago
Attachment #254064 - Flags: review?(mark) → review+
Created attachment 281200 [details]
diff of post-checkout file list and post-make, distclean file list

This is a diff of running "find . -type f" in a srcdir right after checkout, and right after running "make" and then "make distclean".  Note that there are some files that actually got removed, notably the .manifest files are bug 375671.

Comment 25

10 years ago
instgen, staticlib and _tests are easy to remove with GARBAGE_DIRS

I suspect we're just not cleaning nspr at all, which is probably a regression from toolkit-tiers.mk and whatnot... shouldn't be hard to fix.

Do you know what the pstorec.tlh files are?

We should probably add a $(wildcard *.pyc) global garbage rule.
// d:\build\testmoz\clean\mozilla\browser\components\build\pstorec.tlh
//
// C++ source equivalent of Win32 type library C:\WINDOWS\system32\pstorec.dll

It's a typelib header, apparently.  Sounds like we could just add it to GARBAGE.

It looks like there are other scattered files that could be special cased, but not too bad.
Hmm, I'm lost as to which patches are still valid and need to be checked-in... could somebody help? Also, are we sure the patches are still valid after this long of time?
(Assignee)

Updated

10 years ago
Alias: makeclean
(Assignee)

Comment 28

10 years ago
Created attachment 301503 [details] [diff] [review]
Clean up various things

Remove _tests, _javagen, config/system_wrappers, config/buildid, generated *QI.cpp files, run distclean for nspr, clean up libical better, use the unallmakefiles approach for directories/c-sdk (what are the check-in rules here?), clean up copied and linked dirs and files for various mac specific things, and a few other small clean-ups.
Assignee: nobody → jag
Attachment #230605 - Attachment is obsolete: true
Attachment #254064 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #301503 - Flags: superreview?(benjamin)
(Assignee)

Comment 29

10 years ago
Hrm, I forgot to add something for "__.SYMDEF SORTED" (which appears to be Mac only). Maybe in config/rules.mk near where we handle similar stuff for SunOS and OpenVMS?

Hmm, and I just noticed that $(JAVA_GEN_DIR) is added to GARBAGE_DIRS, so that should already have been cleaned. Let me go figure out what's going on there.

On a related note, I'm removing the leading "./" from the XPIDLSRCS line in extensions/typeaheadfind/public/Makefile.in since that causes the .pp file to be created in the wrong place (it combines with "$(JAVA_GEN_DIR)/." to go up a dir).
(Assignee)

Comment 30

10 years ago
Ah, we only add $(JAVA_GEN_DIR) to GARBAGE_DIRS when one of XPIDLSRCS or SDK_XPIDLSRCS isn't empty. Since they're not set at the top level, $objdir/_javagen won't get removed. I guess we could move the GARBAGE_DIRS += $(JAVA_GEN_DIR) line outside the ifneq (but still inside ifdef MOZ_JAVAXPCOM). Or we just special case this like I did in my patch.

Comment 31

10 years ago
Comment on attachment 301503 [details] [diff] [review]
Clean up various things

r=me on the parts that aren't directory/c-sdk. That is an independent project and will need separate review (and branches/tags separately also).
Attachment #301503 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 32

10 years ago
Created attachment 302138 [details] [diff] [review]
Clean up various things minus c-sdk
Attachment #301503 - Attachment is obsolete: true
Attachment #302138 - Flags: superreview+
Attachment #302138 - Flags: approval1.9?
(Assignee)

Comment 33

10 years ago
Bug 416377 covers the directory/c-sdk changes.

Updated

10 years ago
Attachment #302138 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 34

10 years ago
Attachment 302138 [details] [diff] ("Clean up various things minus c-sdk") has been checked in.
Created attachment 302444 [details] [diff] [review]
Cleanup even more

These are some others that are not removed in beta3.

Note that a lot of them come from exporting the files from some other directory, with rules running $(INSTALL). A new variable could be created (for example EXPORT_FILES) that would trigger new rules that would $(INSTALL) the files listed there and remove them at clean time...

Well, people already don't use GARBAGE, why would they use EXPORT_FILES...
Attachment #302444 - Flags: review?(benjamin)
(Assignee)

Comment 36

10 years ago
Looks good to me. I would move the GARBAGE line in xpcom/glue/standalone/Makefile.in above the DEFINES line so it's right next to the $(INSTALL).

I like the idea of EXPORT_FILES, but it'd need a better name.
(Assignee)

Comment 37

10 years ago
Note that .pyc files get created in $(srcdir) (yuck!), so you might want to make that $(srcdir)/Expression.pyc. I also found configobj.pyc in the same place.
Created attachment 302529 [details] [diff] [review]
Cleanup even more, take 2

Updated patch following Peter's comments.

> Note that .pyc files get created in $(srcdir) (yuck!), so you might want to
> make that $(srcdir)/Expression.pyc. I also found configobj.pyc in the same
> place.

That makes sense, that they are in $(srcdir), it's python bytecode, and it's put there by python when Expression.py is imported from Preprocessor.py. Unfortunately, there's no way to prevent bytecode to get generated. Only in 2.6. http://bugs.python.org/issue602345
Attachment #302444 - Attachment is obsolete: true
Attachment #302529 - Flags: review?(benjamin)
Attachment #302444 - Flags: review?(benjamin)
(Assignee)

Comment 39

10 years ago
I understand why they're in $(srcdir) but it's still yucky. It'd be nice if python could be told to put those files in whatever PYTHON_CACHE is set to, or ~/.python/cache/, or something.

Anyway... I think I'd name the two .pyc files explicitly instead of doing *.pyc, but I'll leave that one up to Benjamin.

Updated

10 years ago
Attachment #302529 - Flags: review?(benjamin) → review+
Attachment #302529 - Flags: approval1.9?

Comment 40

10 years ago
Comment on attachment 302529 [details] [diff] [review]
Cleanup even more, take 2

Much appreciated!
Attachment #302529 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in Makefile.in;
/cvsroot/mozilla/Makefile.in,v  <--  Makefile.in
new revision: 1.398; previous revision: 1.397
done
Checking in config/Makefile.in;
/cvsroot/mozilla/config/Makefile.in,v  <--  Makefile.in
new revision: 3.144; previous revision: 3.143
done
Checking in db/morkreader/external/Makefile.in;
/cvsroot/mozilla/db/morkreader/external/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
Checking in embedding/base/standalone/Makefile.in;
/cvsroot/mozilla/embedding/base/standalone/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in extensions/java/xpcom/interfaces/Makefile.in;
/cvsroot/mozilla/extensions/java/xpcom/interfaces/Makefile.in,v  <--  Makefile.in
new revision: 1.19; previous revision: 1.18
done
Checking in intl/unicharutil/util/internal/Makefile.in;
/cvsroot/mozilla/intl/unicharutil/util/internal/Makefile.in,v  <--  Makefile.in
new revision: 1.7; previous revision: 1.6
done
Checking in rdf/util/src/internal/Makefile.in;
/cvsroot/mozilla/rdf/util/src/internal/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
Checking in xpcom/glue/standalone/Makefile.in;
/cvsroot/mozilla/xpcom/glue/standalone/Makefile.in,v  <--  Makefile.in
new revision: 1.39; previous revision: 1.38
done
Keywords: checkin-needed

Comment 42

10 years ago
Created attachment 306952 [details] [diff] [review]
Cleanup even more 2

Some more leftovers (_profile/* and netwerk/dns/src/etld_data.inc).
I've also fixed the xulrunner/installer part (we create some .pc files in there) which has the proper GARBAGE but was never cleaned up.
Attachment #306952 - Flags: review?

Updated

10 years ago
Attachment #306952 - Flags: review? → review?(benjamin)

Comment 43

10 years ago
Comment on attachment 306952 [details] [diff] [review]
Cleanup even more 2

>Index: mozilla/Makefile.in
>===================================================================
>--- mozilla.orig/Makefile.in
>+++ mozilla/Makefile.in
>@@ -49,7 +49,7 @@
> 	$(RM) -rf $(DIST)/include
> 	$(RM) -rf $(DIST)/private
> 	$(RM) -rf $(DIST)/public
>-	$(RM) -rf _tests
>+	$(RM) -rf _javagen _profile _tests

I don't think we want to blow this away on rebuild. The other changes look correct.
Attachment #306952 - Flags: review?(benjamin) → review-
Created attachment 308959 [details] [diff] [review]
cleanup even more 2, v2

This should be what you want
Attachment #306952 - Attachment is obsolete: true
Attachment #308959 - Flags: review?(benjamin)

Updated

10 years ago
Attachment #308959 - Flags: review?(benjamin) → review+

Updated

10 years ago
Attachment #308959 - Flags: approval1.9?
Comment on attachment 308959 [details] [diff] [review]
cleanup even more 2, v2

a1.9+=damons
Attachment #308959 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in Makefile.in;
/cvsroot/mozilla/Makefile.in,v  <--  Makefile.in
new revision: 1.403; previous revision: 1.402
done
Checking in netwerk/dns/src/Makefile.in;
/cvsroot/mozilla/netwerk/dns/src/Makefile.in,v  <--  Makefile.in
new revision: 1.30; previous revision: 1.29
done
Checking in xulrunner/build.mk;
/cvsroot/mozilla/xulrunner/build.mk,v  <--  build.mk
new revision: 1.8; previous revision: 1.7
done
Keywords: checkin-needed

Comment 47

10 years ago
I'm going to call this bug FIXED because it's long and windy. If you discover more uncleanness, please file another bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago10 years ago
Resolution: --- → FIXED

Comment 48

10 years ago
I've created Bug 422991 with more uncleanness.
You need to log in before you can comment on or make changes to this bug.