nsNetUtil.h not included in Visual Studio project Generation

RESOLVED FIXED in mozilla38

Status

--
enhancement
RESOLVED FIXED
4 years ago
7 months ago

People

(Reporter: jkitch, Assigned: jkitch)

Tracking

unspecified
mozilla38

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
nsNetUtil.h lives in netwerk/base/public, which is not included as a library in the generated VS solution.

Is it possible for libraries to be made for public folders if the resulting library is non-empty?

Comment 1

4 years ago
Lots of directories have seen their build system rules collapsed into parent directories. I think netwerk/base/{public,src}/moz.build should be merged into netwerk/base/moz.build.

This is one of those bugs that doesn't really belong to either the build system or the Necko people. It can be reviewed by either camp.

Unless someone in Necko objects, I'll r+ a proper consolidation patch if it arrives in my queue.
(Assignee)

Comment 2

4 years ago
Created attachment 8530092 [details] [diff] [review]
Flatten netwerk/base

The attached patch merges netwerk/base/public and netwerk/base/src and updates other references to these folders using a global search/replace.

Is this something that will need a clobber?
Assignee: nobody → jkitch.bug
Status: NEW → ASSIGNED
Attachment #8530092 - Flags: review?(gps)

Comment 3

4 years ago
Comment on attachment 8530092 [details] [diff] [review]
Flatten netwerk/base

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

You didn't move any files around: you shouldn't need to update all these LOCAL_INCLUDES. You'll also need to prefix a lot of moved moz.build variable definitions with e.g. "public/"
Attachment #8530092 - Flags: review?(gps)
(Assignee)

Comment 4

4 years ago
The moves are present in the raw patch, they just do not show in diff or splinter.  It wouldn't build otherwise.  Other flattening bugs (bug 1041208, bug 1038537) move all files out of the subdirectories, not just the moz.build ones, so the approach in this patch is consistent.


I've also tried the moz.build only approach, but header files in subdirectories are not included in generated VS projects.
Flags: needinfo?(gps)

Comment 5

4 years ago
The patch looks good to me. But you'll need to get necko module review for the file moves. This is definitely not something I feel comfortable granting review to.

Please file a separate bug for headers in subdirectories not getting included in generated VS projects.
Flags: needinfo?(gps)

Comment 6

4 years ago
Disregard the request to file a new bug. Let's figure out what we're doing here first.

Comment 7

4 years ago
If you pop open visualstudio.py, you'll note that we don't handle exports explicitly. That is almost certainly the problem.

I can't recall why we don't do that.
(Assignee)

Comment 8

4 years ago
Comment on attachment 8530092 [details] [diff] [review]
Flatten netwerk/base

Over the past several months, there has been an effort to remove the public/src subdirectories within the Firefox codebase (https://bugzilla.mozilla.org/buglist.cgi?list_id=11706140&short_desc=flatten%20public&query_format=advanced&short_desc_type=allwordssubstr)

I would like to extend this process to netwerk/base.

My motivation for doing this is that some of the header files live in the public subdirectory and this is causing problems with the in-tree IDE project generation tools.

(Note that this patch contains moves that are visible in the raw patch, but not in splinter/diff.  It may also require a clobber).
Attachment #8530092 - Flags: review?(mcmanus)
Comment on attachment 8530092 [details] [diff] [review]
Flatten netwerk/base

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

its moments like this I fear my lack of strong opinions about organization will come back to haunt me :)

based on cross tree precedent I'll f+ the idea.. need a new version though based on :gps's comments.
Attachment #8530092 - Flags: review?(mcmanus) → feedback+
(Assignee)

Comment 10

4 years ago
Unless I am missing something obvious, comment 3 is the only review comment in this bug, which was misled by splinter/diff's inability to show file moves.

The premise of that comment was that only the moz.build files get collapsed into the parent directory.  This is not the approach followed in the patch and other public/src flattening bugs (in them everything gets moved and the public/src directories deleted).

Also, the moz.build only approach regresses the functionality of the automatic project generation tools - netwerk/base/src header files are now excluded as well.
(Assignee)

Comment 11

4 years ago
See comment 10.
Flags: needinfo?(mcmanus)
(In reply to James Kitchener (:jkitch) from comment #11)
> See comment 10.

I made the same error as above. this is fine.
Flags: needinfo?(mcmanus)
Attachment #8530092 - Flags: review+
(Assignee)

Comment 13

4 years ago
Created attachment 8543555 [details] [diff] [review]
part 1: Flatten netwerk/base

rebased
Attachment #8530092 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 8543556 [details] [diff] [review]
part 2: Flatten netwerk/streamconv

For consistency, this applies the flattening process to the public/src directories in netwerk/streamconv.

There is less of an incentive to take this patch as the project generation benefit does not apply here, but the other benefits from bug 946065 (marginally faster builds and history preserving moves) should still apply.

There are two other instances of src directories within netwerk, but both are homes for third party code.
Attachment #8543556 - Flags: review?(mcmanus)
Attachment #8543556 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 15

4 years ago
Comment on attachment 8543555 [details] [diff] [review]
part 1: Flatten netwerk/base

This is a flatten public/src directories patch.

It has r+ from the module owner and passes try, but could you please look through it to verify that I have performed the steps correctly?

My main concern is accidentally trashing the history of the affected directories.
Attachment #8543555 - Flags: feedback?(birunthan)
(Assignee)

Comment 16

4 years ago
Comment on attachment 8543556 [details] [diff] [review]
part 2: Flatten netwerk/streamconv

This is a flatten public/src directories patch.

It has r+ from the module owner and passes try, but could you please look through it to verify that I have performed the steps correctly?

My main concern is accidentally trashing the history of the affected directories.
Attachment #8543556 - Flags: feedback?(birunthan)
(Assignee)

Comment 17

4 years ago
Comment on attachment 8543555 [details] [diff] [review]
part 1: Flatten netwerk/base

See comment 15
Attachment #8543555 - Flags: feedback?(birunthan) → review?(birunthan)
(Assignee)

Comment 18

4 years ago
Comment on attachment 8543556 [details] [diff] [review]
part 2: Flatten netwerk/streamconv

See comment 15
Attachment #8543556 - Flags: feedback?(birunthan) → review?(birunthan)
Comment on attachment 8543556 [details] [diff] [review]
part 2: Flatten netwerk/streamconv

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

Looks good to me.
Attachment #8543556 - Flags: review?(birunthan) → review+
Comment on attachment 8543555 [details] [diff] [review]
part 1: Flatten netwerk/base

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

::: netwerk/cache/moz.build
@@ +47,5 @@
>  
>  FINAL_LIBRARY = 'xul'
>  
>  LOCAL_INCLUDES += [
> +    '../base',

It would be nice to change this and other similar paths to absolute paths, but r+ either way.
Attachment #8543555 - Flags: review?(birunthan) → review+
(Assignee)

Comment 21

4 years ago
Created attachment 8546972 [details] [diff] [review]
part 1: Flatten netwerk/base

rebased
Attachment #8543555 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 8546975 [details] [diff] [review]
Part 3 : Use absolute paths for /netwerk moz.build files

Comment 20 suggested converting the relative paths used in /netwerk moz.build files to absolute ones.

Worth taking?
Attachment #8546975 - Flags: review?(gps)
Comment on attachment 8546975 [details] [diff] [review]
Part 3 : Use absolute paths for /netwerk moz.build files

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

This isn't strictly required. But if it's what you want, go for it.
Attachment #8546975 - Flags: review?(gps) → review+
(Assignee)

Comment 24

4 years ago
Created attachment 8551740 [details] [diff] [review]
part 1: Flatten netwerk/base

rebased
Attachment #8546972 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
Created attachment 8551743 [details] [diff] [review]
Part 3 : Use absolute paths for /netwerk moz.build files

Rebased

At the very least it will make future automatic code relocations slightly easier.
Attachment #8546975 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59cbf59402ab

Note that these are "flatten public/src" patches.  They move .cpp, .h and .idl files into parent directories and reduces the number of moz.build files.
Keywords: checkin-needed
(In reply to James Kitchener from comment #2)
> Is this something that will need a clobber?

The big problem I have noticed when moving files is if they are still referenced in SOURCES in the same moz.build file. If they are referenced in UNIFIED_SOURCES or if they are moved to a different moz.build file then I haven't noticed a problem.
https://hg.mozilla.org/mozilla-central/rev/4ec37503ea6a
https://hg.mozilla.org/mozilla-central/rev/3de3af63a33f
https://hg.mozilla.org/mozilla-central/rev/f778e224ff27
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Updated

7 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.