Address issue #3312 #3356

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

mattness commented Jun 1, 2012

  1. Add step in configure script to generate config.wxi preprocessor include
    for WiX installer (on windows only).
  2. Update nodemsi.sln and .wixproj to include support for x64 platform
  3. Update product.wxs:
    • add preprocessor directive to include config.wxi generated by the
      configure script.
    • update the Id value for the "Program Files" Directory element to
      use the preprocessor variable in config.wxi to correctly select
      either ProgramFilesFolder or ProgramFiles64Folder.
    • remove hard-coded platform from the Package element. MSI platform
      will be automatically detected based on MSBuild's Platform property.
      (This was already supported in the Wix MSBuild targets, we just weren't
      taking advantage of it.)
  4. Update vcbuild.bat to set MSBuild's Platform property appropriately,
    defaulting to x86 if not explicitly supplied by the user.
@mattness mattness Address issue #3312
1. Add step in configure script to generate config.wxi preprocessor include
for WiX installer (on windows only).
2. Update nodemsi.sln and .wixproj to include support for x64 platform
3. Update product.wxs:
     - add preprocessor directive to include config.wxi generated by the
       configure script.
     - update the Id value for the "Program Files" Directory element to
       use the preprocessor variable in config.wxi to correctly select
       either ProgramFilesFolder or ProgramFiles64Folder.
     - remove hard-coded platform from the Package element.  MSI platform
       will be automatically detected based on MSBuild's Platform property.
       (This was already supported in the Wix MSBuild targets, we just weren't
       taking advantage of it.)
4. Update vcbuild.bat to set MSBuild's Platform property appropriately,
defaulting to x86 if not explicitly supplied by the user. Note that creating
an x64 build requires that vcbuild.bat be run from a VS 64-bit command prompt.
e54fad0

mattness commented Jun 1, 2012

I have signed the CLA.

piscisaureus was assigned Jun 1, 2012

Owner

bnoordhuis commented Jun 1, 2012

@piscisaureus Review?

Member

piscisaureus commented Jun 1, 2012

Generally looks good, but I'm not really down with the changes to configure. Currently building on windows doesn't require running ./configure and I'd like to keep it that way. Is it possible to configure the program files folder another way?

@piscisaureus Building on Windows has been calling to ./configure ever since process.config was implemented.

Member

piscisaureus commented Jun 1, 2012

@mattness @TooTallNate

Still, I think think it's not necessary to generate/use a config file. You can just add the ProgramFilesFolderId constant to each of the 4 groups in the nodemsi.wixproj file. (e.g. here: https://github.com/mattness/node/blob/e54fad0394b097f872b95f3f79cfdc9ddc7c55f6/tools/msvs/msi/nodemsi.wixproj#L19)

mattness commented Jun 1, 2012

@piscisaureus Sure, it's possible to configure it a different way. For example, we could write the .wxi as part of vcbuild.bat, or pass the ProgramFilesFolderId property to msbuild on the command line. But as @TooTallNate mentioned, vcbuild.bat was already running configure, so that seemed like the most logical place to put it.

I'm happy to change it however you'd like, just let me know what would be preferred.

Member

piscisaureus commented Jun 1, 2012

@mattness I think we just encountered a race condition :-). My preferred way is outlined two comments up.

mattness commented Jun 1, 2012

@piscisaureus Yep, that would work too, and is actually simpler. I'll make the change.

@mattness mattness Back out changes to configure, .gitignore. Implement in .wixproj instead
1. Rollback changes to configure that write .wxi file
2. Rollback changes to .gitignore that add /config.wxi to the ignore list
3. Remove the preprocessor include for config.wxi from product.wxs
4. Add ProgramFilesFolderId to the DefineConstants property for each
   configuration/platform's property group with the appropriate value
   (ProgramFilesFolder for x86 builds, ProgramFiles64Folder for x64 builds)
c7daedf
Member

piscisaureus commented Jun 2, 2012

Landed. Thanks, @mattness

@exinfinitum exinfinitum pushed a commit to exinfinitum/node that referenced this pull request Oct 16, 2015

@dohse @misterdjules dohse + misterdjules test: port domains regression test from v0.10
f2a45ca contained a test for a
regression that had been introduced by the original change that
77a10ed ported. While
77a10ed did not contain that
regression, the test that f2a45ca
contained should still be in the code base to prevent any regression
from happening in the future.

Original message for the commit that contained the test:

  domains: fix stack clearing after error handled

  caeb677 introduced a regression where
  the domains stack would not be cleared after an error had been handled
  by the top-level domain.

  This change clears the domains stack regardless of the position of the
  active domain in the stack.

  PR: #9364
  PR-URL: nodejs#9364
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Julien Gilli <julien.gilli@joyent.com>

PR: #3356
PR-URL: nodejs/node#3356
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
642928b

@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Nov 5, 2015

@dohse @jasnell dohse + jasnell test: port domains regression test from v0.10
f2a45ca contained a test for a
regression that had been introduced by the original change that
77a10ed ported. While
77a10ed did not contain that
regression, the test that f2a45ca
contained should still be in the code base to prevent any regression
from happening in the future.

Original message for the commit that contained the test:

  domains: fix stack clearing after error handled

  caeb677 introduced a regression where
  the domains stack would not be cleared after an error had been handled
  by the top-level domain.

  This change clears the domains stack regardless of the position of the
  active domain in the stack.

  PR: #9364
  PR-URL: nodejs#9364
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Julien Gilli <julien.gilli@joyent.com>

PR: #3356
PR-URL: nodejs/node#3356
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
7cad182

@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Nov 5, 2015

@dohse @rvagg dohse + rvagg test: port domains regression test from v0.10
f2a45ca contained a test for a
regression that had been introduced by the original change that
77a10ed ported. While
77a10ed did not contain that
regression, the test that f2a45ca
contained should still be in the code base to prevent any regression
from happening in the future.

Original message for the commit that contained the test:

  domains: fix stack clearing after error handled

  caeb677 introduced a regression where
  the domains stack would not be cleared after an error had been handled
  by the top-level domain.

  This change clears the domains stack regardless of the position of the
  active domain in the stack.

  PR: #9364
  PR-URL: nodejs#9364
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Julien Gilli <julien.gilli@joyent.com>

PR: #3356
PR-URL: nodejs/node#3356
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
e2b8393
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment