Closed
Bug 1157420
Opened 9 years ago
Closed 9 years ago
mach should generate VS2013 project files for build-backend VisualStudio
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox42 fixed)
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: vlad, Assigned: vlad)
References
Details
Attachments
(1 file)
7.12 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
Currently it generates Visual Studio 2010 files, which we don't even build with. The only thing that needs to change is the header to mozilla.sln, should be: Microsoft Visual Studio Solution File, Format Version 12.00 # Visual Studio 2013 VisualStudioVersion = 12.0.30110.0 If possible, I would also suggest at the same time putting all projects inside a msvc/projects subdir, so: msvc/mozilla.sln msvc/projects/library_accessible_generic.vcxproj msvc/projects/library_accessible_html.vcxproj etc.; the project files aren't really useful by themselves, and there are a ton of them in the msvc dir.
Assignee | ||
Comment 1•9 years ago
|
||
Oh -- the project files themselves are also marked as using the VS 2010 toolchain; we can upgrade that as well, though it doesn't really matter since internally they use our build system to build anyway.
Assignee | ||
Comment 2•9 years ago
|
||
(I should really make a separate bug, but the "js" binary project should go into the "Binaries" folder in the solution as well, not the toplevel)
Assignee | ||
Comment 3•9 years ago
|
||
This patch does three things: - Generates VS2013 solution and project files (project files don't matter, but it gets rid of the "(Visual Studio 2010)" suffix on everything) - Changes project ID generation to use the generated name ("library_js", "binary_js") instead of the base name ("js") which was causing collisions - Moves all vcxproj files into a "projects" subdirectory of msvc, making the msvc dir cleaner and making it more obvious what to open
Attachment #8631626 -
Flags: review?(nfroyd)
Comment 4•9 years ago
|
||
Comment on attachment 8631626 [details] [diff] [review] Use VS 2013 and clean up dir structure Review of attachment 8631626 [details] [diff] [review]: ----------------------------------------------------------------- I'm not really a VS project expert, so I'm mostly going by "hey, somebody says it works, r+". That being said, I have a few questions/comments. ::: python/mozbuild/mozbuild/backend/visualstudio.py @@ +43,5 @@ > return '12.00' > elif version == '2012': > + return '12.00' > + elif version == '2013': > + return '12.00' Were we returning a completely bogus value for 2012 previously, then? @@ +469,4 @@ > > def _write_vs_project(self, out_dir, basename, name, **kwargs): > root = '%s.vcxproj' % basename > + project_id = get_id(basename.encode('utf-8')) Note to drive-by reviewers: yes, this code does what the patch description says, despite looking like it contradicts the patch description due to bad variable names. =/ @@ +539,4 @@ > ig = project.appendChild(doc.createElement('ImportGroup')) > ig.setAttribute('Label', 'PropertySheets') > i = ig.appendChild(doc.createElement('Import')) > + i.setAttribute('Project', '..\\mozilla.props') Ugh, this is not great that we have this hidden dependence on the value of _projsubdir. But I'm not sure it's worth threading _projsubdir all the way down here...maybe if we turned this into a non-static method, then we could have it implicitly available? Wanna file a followup?
Attachment #8631626 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4) > Comment on attachment 8631626 [details] [diff] [review] > Use VS 2013 and clean up dir structure > > Review of attachment 8631626 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not really a VS project expert, so I'm mostly going by "hey, somebody > says it works, r+". That being said, I have a few questions/comments. > > ::: python/mozbuild/mozbuild/backend/visualstudio.py > @@ +43,5 @@ > > return '12.00' > > elif version == '2012': > > + return '12.00' > > + elif version == '2013': > > + return '12.00' > > Were we returning a completely bogus value for 2012 previously, then? I think so -- the solution version didn't go up. But it didn't matter, since 2010 was hardcoded :) > @@ +539,4 @@ > > ig = project.appendChild(doc.createElement('ImportGroup')) > > ig.setAttribute('Label', 'PropertySheets') > > i = ig.appendChild(doc.createElement('Import')) > > + i.setAttribute('Project', '..\\mozilla.props') > > Ugh, this is not great that we have this hidden dependence on the value of > _projsubdir. But I'm not sure it's worth threading _projsubdir all the way > down here...maybe if we turned this into a non-static method, then we could > have it implicitly available? Wanna file a followup? Eh, it's a hidden dependency only in that it needs to be a single dir. I can just put a comment before _projsubdir to say "this must be a single directory name, not a path" as well as point to the mozilla.props thing and leave it at that. I mean, noone's going to change it, nor can they without mucking with the backend code.
Assignee | ||
Comment 7•9 years ago
|
||
I fixed the "mozilla.props" issue by just creating mozilla.props in the project subdir.. so no hidden "..\" dependency.
Assignee: nobody → vladimir
https://hg.mozilla.org/mozilla-central/rev/829844959271
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•