Closed Bug 1100184 Opened 10 years ago Closed 9 years ago

nsNetUtil.h not included in Visual Studio project Generation

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla38

People

(Reporter: jkitch, Assigned: jkitch)

References

Details

Attachments

(3 files, 4 obsolete files)

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?
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.
Attached patch Flatten netwerk/base (obsolete) — Splinter Review
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 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)
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)
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)
Disregard the request to file a new bug. Let's figure out what we're doing here first.
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.
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+
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.
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)
Attached patch part 1: Flatten netwerk/base (obsolete) — Splinter Review
rebased
Attachment #8530092 - Attachment is obsolete: true
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+
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)
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)
Comment on attachment 8543555 [details] [diff] [review]
part 1: Flatten netwerk/base

See comment 15
Attachment #8543555 - Flags: feedback?(birunthan) → review?(birunthan)
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+
Attached patch part 1: Flatten netwerk/base (obsolete) — Splinter Review
rebased
Attachment #8543555 - Attachment is obsolete: true
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+
rebased
Attachment #8546972 - Attachment is obsolete: true
Rebased

At the very least it will make future automatic code relocations slightly easier.
Attachment #8546975 - Attachment is obsolete: true
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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: