845556 - reorganize the NSS HG repository

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

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.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: remove unnecessary makefile

Comment 2

[Kai Engert (:KaiE:)]

Attachment: move security/dbm to dbm

Comment 3

[Kai Engert (:KaiE:)]

Attachment: move security/dbm/include files to dbm/include

Comment 4

[Kai Engert (:KaiE:)]

Attachment: move security/dbm/src to dbm/src

Comment 5

[Kai Engert (:KaiE:)]

Attachment: move security/dbm/tests to dbm/tests

Comment 6

[Kai Engert (:KaiE:)]

Attachment: move security/dbm/config to dbm/config

Comment 7

[Kai Engert (:KaiE:)]

Attachment: move security/coreconf to coreconf

Comment 8

[Kai Engert (:KaiE:)]

Attachment: patch: adjust makefiles for new hierarchy

Comment 9

[Kai Engert (:KaiE:)]

Attachment: move security/nss/* to toplevel

Comment 10

[Kai Engert (:KaiE:)]

Attachment: move dbm to lib/dbm

Comment 11

[Kai Engert (:KaiE:)]

Attachment: patch: fix tests

Comment 12

[Kai Engert (: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 13

[Wan-Teh Chang]

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.

Comment 14

[Wan-Teh Chang]

Comment on attachment 718688 [details] [diff] [review]
patch: fix tests

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

r=wtc.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: add this .hgignore file to each of nspr, nss and jss

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.

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 718996 [details]
add this .hgignore file to each of nspr, nss and jss

Note that it works. Just asking for approval.

Comment 17

[Kai Engert (:KaiE:)]

(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

Comment 18

[Kai Engert (:KaiE:)]

Attachment: updated patch

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.

Comment 19

[Kai Engert (:KaiE:)]

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 20

[Wan-Teh Chang]

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?

Comment 21

[Kai Engert (:KaiE:)]

(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 22

[Wan-Teh Chang]

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 =.

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 25

[Kai Engert (:KaiE:)]

(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?

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.)

Comment 27

[Kai Engert (:KaiE:)]

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.

Comment 28

[Kai Engert (:KaiE:)]

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.

Comment 29

[Kai Engert (:KaiE:)]

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.

Comment 30

[Kai Engert (:KaiE:)]

Attachment: v13

Patch addressing latest requests (top of the removals/additions).

Comment 31

[Kai Engert (:KaiE:)]

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.

Comment 32

[Kai Engert (:KaiE:)]

(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.

Comment 33

[Kai Engert (:KaiE:)]

fixed

Comment 34

[Kai Engert (:KaiE:)]

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

Comment 35

[Kai Engert (:KaiE:)]

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

Comment 36

[Wan-Teh Chang]

(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.

Comment 37

[Kai Engert (:KaiE:)]

(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

Comment 38

[Wan-Teh Chang]

Attachment: Make the NSS build system treat coreconf as an integral part of NSS

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

Comment 39

[Kai Engert (:KaiE:)]

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.)

Comment 40

[Wan-Teh Chang]

Please use my "Full Name <email address>" in the -u option
for hg commit.  Thanks.

Comment 41

[Kai Engert (:KaiE:)]

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