reorganize the NSS HG repository

RESOLVED FIXED in 3.15

Status

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

3.14.3
3.15
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments, 1 obsolete attachment)

281 bytes, patch
wtc
: review+
Details | Diff | Splinter Review
412 bytes, patch
wtc
: review+
Details | Diff | Splinter Review
476 bytes, patch
wtc
: review+
Details | Diff | Splinter Review
817 bytes, patch
wtc
: review+
Details | Diff | Splinter Review
317 bytes, patch
wtc
: review+
Details | Diff | Splinter Review
325 bytes, patch
wtc
: review+
Details | Diff | Splinter Review
13.73 KB, patch
wtc
: review+
Details | Diff | Splinter Review
54.79 KB, patch
wtc
: review+
Details | Diff | Splinter Review
422.68 KB, patch
wtc
: review+
Details | Diff | Splinter Review
4.79 KB, patch
wtc
: review+
Details | Diff | Splinter Review
921 bytes, patch
wtc
: review+
Details | Diff | Splinter Review
39 bytes, text/plain
wtc
: review+
Details
v13
62.29 KB, patch
wtc
: review+
Details | Diff | Splinter Review
2.73 KB, patch
kaie
: review+
Details | Diff | Splinter Review
It has been suggested to reorganize the layout of the NSS repository.

I will attach a series of files that shows the suggested changes in order.

After the reorg, the repo will contain the following files in the toplevel:

./Makefile
./manifest.mn
./trademarks.txt
./COPYING

And the following directories:

./coreconf
./pkg
./lib
./doc
./coverage
./tests
./cmd

Both dbm directory will have been merged into lib/dbm.
Assignee: nobody → kaie
Comment on attachment 718685 [details] [diff] [review]
patch: adjust makefiles for new hierarchy

Wan-Teh already gave this comment by email:

> 1. The manifest.mn files inside dbm have these lines:
>     VPATH = .
> They should be simply removed.
>
> 2. security/nss/Makefile
> There original lines were commented out. They should be removed.

I agree. Will do so on final checkin.
Comment on attachment 718685 [details] [diff] [review]
patch: adjust makefiles for new hierarchy

Review of attachment 718685 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. I have a question, and some suggested changes. Thanks.

::: dbm/Makefile
@@ +48,1 @@
>  # (7) Execute "local" rules. (OPTIONAL).                              #

At the beginning of this file, we have

13 ifdef NSS_DISABLE_DBM
14 DIRS =
15 endif

That should be removed. (That is a pre-existing problem. It
should not have been added in the first place.)

@@ +48,4 @@
>  # (7) Execute "local" rules. (OPTIONAL).                              #
>  #######################################################################
>  
> +export:: private_export

Why is this change necessary?

::: dbm/include/manifest.mn
@@ +8,2 @@
>  
> +VPATH  = .

Remove this because searching for files in the current directory (.)
is the default behavior.

::: dbm/src/manifest.mn
@@ +8,2 @@
>  
> +VPATH  = .

Remove this because searching for files in the current directory (.)
is the default behavior.

::: security/nss/Makefile
@@ +44,4 @@
>  # (7) Execute "local" rules. (OPTIONAL).                              #
>  #######################################################################
>  
> +#nss_build_all: build_coreconf build_nspr build_dbm all

Remove the three commented-out lines:

#nss_build_all: ...
#nss_clean_all: ...
#nss_RelEng_bld: ...

::: security/nss/lib/manifest.mn
@@ +8,5 @@
> +DIRS =
> +
> +ifndef NSS_DISABLE_DBM
> +DIRS += dbm
> +endif

Your solution works, but I recommend that you use a solution
similar to SQLITE_SRCDIR and ZLIB_SRCDIR.

1. In security/nss/lib/Makefile, section 4, add

ifndef NSS_DISABLE_DBM
DBM_SRCDIR = dbm  # Add the dbm directory to DIRS.
endif

2. In security/nss/lib/manifest.mn, add $(DBM_SRCDIR) before softoken.
Attachment #718685 - Flags: review+
Comment on attachment 718688 [details] [diff] [review]
patch: fix tests

Review of attachment 718688 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.
Attachment #718688 - Flags: review+
Attachment #718687 - Flags: review+
Attachment #718686 - Flags: review+
Attachment #718683 - Flags: review+
Attachment #718677 - Flags: review+
Attachment #718678 - Flags: review+
Attachment #718679 - Flags: review+
Attachment #718680 - Flags: review+
Attachment #718681 - Flags: review+
Attachment #718682 - Flags: review+
I'd like to add this file to the toplevel of each HG repository, to ensure that hg commands will ignore build output files, and also editor backup files.
Attachment #718996 - Flags: review?(wtc)
Comment on attachment 718996 [details]
add this .hgignore file to each of nspr, nss and jss

Note that it works. Just asking for approval.
Target Milestone: --- → 3.14.4
(In reply to Wan-Teh Chang from comment #13)
> @@ +48,4 @@
> >  # (7) Execute "local" rules. (OPTIONAL).                              #
> >  #######################################################################
> >  
> > +export:: private_export
> 
> Why is this change necessary?


Without it, I get an error message:

make[3]: Entering directory `/pd/moz/nss/new-hg/nss/lib/dbm/src'
gcc -o Linux3.7_x86_64_glibc_PTH_64_DBG.OBJ/h_bigkey.o -c -g -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE -fPIC -DLINUX2_1  -Wall -Werror-implicit-function-declaration -Wno-switch -pipe -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -DDEBUG -UNDEBUG -DDEBUG_kaie -D_REENTRANT -DUSE_UTIL_DIRECTLY -DSTDC_HEADERS -DHAVE_STRERROR -DHAVE_SNPRINTF -DHAVE_SYS_CDEFS_H -DMEMMOVE -D__DBINTERFACE_PRIVATE  -I../../../../dist/Linux3.7_x86_64_glibc_PTH_64_DBG.OBJ/include -I../../../../dist/public/dbm -I../../../../dist/private/dbm -I../../../../dbm/include  h_bigkey.c
h_bigkey.c:71:18: fatal error: hash.h: No such file or directory
Posted patch updated patch (obsolete) — Splinter Review
I have addressed all your change requests from comment 13.

But I had noticed that my earlier commands will move all dbm files twice, making the history unnecessarily more complicated.

Therefore I have simply reordered the commands, but keeping all actions identical.

Here is the new series of actions:
hg rename security/nss/* .
hg rename security/coreconf .
hg remove dbm/Makefile.in
hg rename dbm lib/dbm
hg rename security/dbm/config security/dbm/Makefile security/dbm/manifest.mn lib/dbm/
hg rename security/dbm/include/Makefile security/dbm/include/manifest.mn lib/dbm/include/
hg rename security/dbm/tests/Makefile lib/dbm/tests/
hg rename security/dbm/src/* lib/dbm/src/

I've merged the two changing patches into one.
And I'm attaching it, including that changes that you had requested.
even better:

hg -q rename security/nss/* .
hg -q rename security/coreconf .
hg -q remove dbm/Makefile.in dbm/include/Makefile.in dbm/src/Makefile.in dbm/tests/Makefile.in
hg -q remove dbm/include/Makefile.win dbm/src/Makefile.win
hg -q rename dbm lib/dbm
hg -q rename security/dbm/config security/dbm/Makefile security/dbm/manifest.mn lib/dbm/
hg -q rename security/dbm/include/Makefile security/dbm/include/manifest.mn lib/dbm/include/
hg -q rename security/dbm/tests lib/dbm/
hg -q rename security/dbm/src/* lib/dbm/src/

This avoids moving files that will be removed anyway.
It simplifies the attached patch, so it doesn't need to list the removed files.

I propose that I will commit these changes to the repo in two parts:
- commit 1 with all the moves and removals, and use an appropriate checkin
  comment, that will people scare away from click it, something like
    "reorganize NSS directory layout, moves and removals only, very large changeset"
- commit 2 that adjusts the makefiles, using the attached patch
Comment on attachment 718996 [details]
add this .hgignore file to each of nspr, nss and jss

r=wtc. You also need

  *DBG.OBJD/*

which is only used on Windows.

Did you migrate the .cvsingore files in the source trees?
Attachment #718996 - Flags: review?(wtc) → review+
(In reply to Wan-Teh Chang from comment #20)
> 
> Did you migrate the .cvsingore files in the source trees?

No, that's a TODO
Comment on attachment 719045 [details] [diff] [review]
updated patch

Review of attachment 719045 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. I suggest two tweaks. Thanks.

::: lib/dbm/Makefile
@@ +43,5 @@
>  #######################################################################
>  # (7) Execute "local" rules. (OPTIONAL).                              #
>  #######################################################################
>  
> +export:: private_export

Please remove this line. Then, edit lib/dbm/src/config.mk and update

  INCLUDES += -I$(CORE_DEPTH)/../dbm/include

Note: "export:: private_export" is a good solution, but it is better
to add it to lib/dbm/include/Makefile instead. If you use this solution,
please remove the line

  INCLUDES += -I$(CORE_DEPTH)/../dbm/include

from lib/dbm/src/config.mk. Since dbm is old code, I'd probably just
update the INCLUDES line in lib/dbm/src/config.mk.

::: lib/manifest.mn
@@ +15,5 @@
>  #  ssl
>  #  smime
>  #  ckfw (builtins module)
>  #  crmf jar (not dll's)
> +DIRS += util freebl $(SQLITE_SRCDIR) $(DBM_SRCDIR) softoken \

Please change += back to =.
Attachment #719045 - Flags: review+
(In reply to Kai Engert (:kaie) from comment #18)
> But I had noticed that my earlier commands will move all dbm files twice,
> making the history unnecessarily more complicated.

...

> I've merged the two changing patches into one.
> And I'm attaching it, including that changes that you had requested.

Please just merge all of these changesets together before committing them. It is easy to do with the "hg histedit" command:

  hg histedit <first revision id>

change all the "pick" to "fold" except for the first "pick" in the file that comes up in $EDITOR.

This way, the disruption to the history/blame will be minimal.
(In reply to Brian Smith (:bsmith) from comment #23)
> Please just merge all of these changesets together before committing them.

Sorry, I mean before *pushing* them. That is, histedit should be run after committing them and before pushing them.
(In reply to Brian Smith (:bsmith) from comment #23)
> Please just merge all of these changesets together before committing them.
> It is easy to do with the "hg histedit" command:


I don't understand why.

The proposal is two use just two commits.
What's the benefit of merging them into one?
(In reply to Kai Engert (:kaie) from comment #25)
> I don't understand why.
> 
> The proposal is two use just two commits.
> What's the benefit of merging them into one?

The policy we use in mozilla-central is that every commit in mozilla-central must build and run the tests correctly, whenever possible. This allows bisecting to work best. I understand that the output of the cvs2hg conversion from CVS -> Mercurial breaks this rule, out of necessity, because we split out NSPR. However, if we can avoid breaking the rule in this bug, then we should do so.

(BTW if you are using Mercurial queues for this, you can accomplish the same effect with qfold before pushing.)
The justification given in comment 26 doesn't seem to apply to the NSPR/NSS repos, and I consider the benefit of having the real changes in a small readable patch as more important.
Wan-Teh, I tried to understand comment 22, but it's not clear.
What I tried is failing

What I did:

- in lib/dbm/Makefile I reverted the change and removed the line
  export:: private_export

- in lib/dbm/src/config.mk I removed the line
  INCLUDES += -I$(CORE_DEPTH)/../dbm/include

- in lib/manifest.mn I changed
  DIRS += 
  to
  DIRS =

Then I rebuilt, and I still get the build failure mentioned in comment 17.
Never mind, ignore comment 28. I missed the part that you want the
  export:: private_export
line added to lib/dbm/include/Makefile.

That works.
Posted patch v13Splinter Review
Patch addressing latest requests (top of the removals/additions).
Attachment #719207 - Flags: review+
Attachment #719045 - Attachment is obsolete: true
We have received the new HG repos, and I've checked in this change for initial testing.
I was able to, that means, I've correctly been given permission to push.

The links to the checkins are:
move: https://hg.mozilla.org/projects/nss/rev/6c43fe3ab5dd (avoid to click, large).
adjust: https://hg.mozilla.org/projects/nss/rev/b4f684c82747

At this time, the links don't show the detail patch, but I assume that's a temporary problem, have already asked Mozilla IT to look into it.

Please keep this bug open. I still need to land the .hgignore files.
(In reply to Kai Engert (:kaie) from comment #31)
> adjust: https://hg.mozilla.org/projects/nss/rev/b4f684c82747
> 
> At this time, the links don't show the detail patch, but I assume that's a
> temporary problem, have already asked Mozilla IT to look into it.

My fault. I had accidentally commited an empty changeset.

This is the fixed one:
  https://hg.mozilla.org/projects/nss/rev/c1590d7c2d79
and it looks good.
fixed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Windows tests were not yet working. I checked in the patch below as a fix for the windows testing bustage.
https://hg.mozilla.org/projects/nss/rev/566aa414fe0c
Android tests were not yet working. I checked in the patch below as a fix for the Android testing bustage.
https://hg.mozilla.org/projects/nss/rev/4c6119e34aa0
(In reply to Kai Engert (:kaie) from comment #35)
> Android tests were not yet working. I checked in the patch below as a fix
> for the Android testing bustage.
> https://hg.mozilla.org/projects/nss/rev/4c6119e34aa0

You should keep the >> $(RTSH) aligned on the right.
(In reply to Wan-Teh Chang from comment #36)
> (In reply to Kai Engert (:kaie) from comment #35)
> > Android tests were not yet working. I checked in the patch below as a fix
> > for the Android testing bustage.
> > https://hg.mozilla.org/projects/nss/rev/4c6119e34aa0
> 
> You should keep the >> $(RTSH) aligned on the right.

done:
https://hg.mozilla.org/projects/nss//rev/645133b5b37e2220d43e689800771dabc296bafb
1. Add coreconf to the DIRS list in the top-level manifest.mn file.

2. In the top-level Makefile, remove the build_coreconf and
clobber_coreconf targets.

This requires removing the use of $(NSINSTALL) in the top-level
Makefile to create the NSPR objdir, because now nss_build_all
will build nspr before building coreconf (which provides nsinstall).
I replaced $(NSINSTALL) -D with mkdir -p.

I also removed the unused moz_import target:

http://mxr.mozilla.org/mozilla-central/search?string=moz_import&case=on&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
Attachment #733503 - Flags: review?(kaie)
Attachment #733503 - Attachment description: Make the NSS build system consider coreconf as an integral part of NSS → Make the NSS build system treat coreconf as an integral part of NSS
Comment on attachment 733503 [details] [diff] [review]
Make the NSS build system treat coreconf as an integral part of NSS

r=kaie

(I offer to check it in for you, after I configure some of the NSS build machines, so I can combine some build cycles.)
Attachment #733503 - Flags: review?(kaie) → review+
Please use my "Full Name <email address>" in the -u option
for hg commit.  Thanks.
Comment on attachment 733503 [details] [diff] [review]
Make the NSS build system treat coreconf as an integral part of NSS

https://hg.mozilla.org/projects/nss/rev/ad733dae248d
Attachment #733503 - Flags: checked-in+
Target Milestone: 3.14.4 → 3.15
You need to log in before you can comment on or make changes to this bug.