Windows: Add missing MSVC dependencies #16445

UK992 · 2017-04-13T20:14:11Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #16422

This change is

highfive · 2017-04-13T20:14:14Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/build_commands.py, python/servo/package_commands.py

aneeshusa

Looks good so far! I'd also like to know a) what are the licensing terms for these libraries, and b) are we OK to redistribute these libraries as part of our nightly builds?

aneeshusa · 2017-04-14T15:10:52Z

python/servo/package_commands.py

+            "libsslMD.dll"
+            "msvcp140.dll",
+            "vcruntime140.dll",
+            "api-ms-win-crt-runtime-l1-1-0.dll",


nit: move to the top of the list for alphabetical order

aneeshusa · 2017-04-14T15:10:52Z

python/servo/build_commands.py

+                    if os.environ["VisualStudioVersion"] == "14.0":
+                        vs_dir = os.environ["VS140COMNTOOLS"]
+                        vs_dll_dir = path.join(vs_dir, "..", "IDE", "Remote Debugger", os.environ["PLATFORM"].lower())
+                        for vs_dll in ["api-ms-win-crt-runtime-l1-1-0.dll"]:


No need to make this a loop for just one item, we can always re-add the loop in the future if necessary.

aneeshusa · 2017-04-14T15:10:52Z

python/servo/build_commands.py

+                        "api-ms-win-crt-runtime-l1-1-0.dll",
+                    ]
+                    for msvc_dll in msvc_deps:
+                        dll = subprocess.Popen(["where", msvc_dll], stdout=subprocess.PIPE)


Can this use distutils.spawn.find_executable instead? If not, please add a comment about why that doesn't work.

distutils.spawn.find_executable doesn't work on dlls, but i found alternative ctypes.util.find_library.

aneeshusa · 2017-04-14T17:56:52Z

python/servo/build_commands.py

+                    if os.environ.get("VS140COMNTOOLS", ""):
+                        vs_dir = os.environ["VS140COMNTOOLS"]
+                        vs_dll_dir = path.join(vs_dir, "..", "IDE", "Remote Debugger", os.environ["PLATFORM"].lower())
+                        os.environ['PATH'] = os.environ['PATH'] + os.pathsep + vs_dll_dir


I don't like modifying environment variables in process, it can be confusing and potentially can cause memory leaks, etc. I believe you had an approach that involved copying before - can we use that instead? Or maybe symlinking?

aneeshusa · 2017-04-14T17:56:52Z

python/servo/package_commands.py

+            "msvcp140.dll",
+            "vcruntime140.dll",
+        ]
+        [shutil.copy(path.join(binary_path, d), destination) for d in deps]


Since the return value of the list comprehension is not used, this is better written as a regular for loop.

aneeshusa · 2017-04-14T17:56:52Z

python/servo/build_commands.py

@@ -324,6 +324,20 @@ def build(self, target=None, release=False, dev=False, jobs=None,
                    for ssl_lib in ["libcryptoMD.dll", "libsslMD.dll"]:
                        shutil.copy(path.join(env['OPENSSL_LIB_DIR'], "../bin" + msvc_x64, ssl_lib),
                                    servo_exe_dir)
+                    if os.environ.get("VS140COMNTOOLS", ""):


This could be clearer as if "VS140COMNTOOLS" in os.environ:.

aneeshusa · 2017-04-14T17:56:52Z

python/servo/build_commands.py

+                        "vcruntime140.dll",
+                    ]
+                    for msvc_dll in msvc_deps:
+                        from ctypes.util import find_library


I'd move this outside the for loop to avoid importing for each dll.

aneeshusa · 2017-04-15T03:44:59Z

Code changes look good to me, needs a squash.
I'd like answers to the licensing questions before approving this.

UK992 · 2017-04-15T19:13:07Z

@aneeshusa it's ok to redistribute these libraries (link), also i updated code to take these libs from same locations as firefox does.

bors-servo · 2017-04-17T03:29:30Z

☔ The latest upstream changes (presumably #16490) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-04-17T15:58:52Z

Error running mach:
    ['build', '-d', '-v']
The error occurred in the implementation of the invoked mach command.
This should never occur and is likely a bug in the implementation of that
command. Consider filing a bug for this issue.
If filing a bug, please include the full output of mach, including this error
message.
The details of the failure are as follows:
WindowsError: [Error 2] The system cannot find the file specified: u'C:\\projects\\servo\\target\\debug\\api-ms-win-crt-runtime-l1-1-0.dll'
  File "C:\projects\servo\python\servo\build_commands.py", line 358, in build
    os.chmod(servo_dir_dll, stat.S_IWUSR)
Command exited with code 1

larsbergstrom · 2017-04-23T15:34:30Z

@aneeshusa This looks good to me - is it ready for r+?

aneeshusa

I've left some comments and questions on this due to the new implementation.

aneeshusa · 2017-04-23T15:58:55Z

python/servo/package_commands.py

-            shutil.make_archive(path.join(dir_to_msi, "Servo"), "zip", dir_to_temp)
-            print("Packaged Servo into " + path.join(dir_to_msi, "Servo.zip"))
+            zip_path = path.join(dir_to_msi, "Servo.zip")
+            import zipfile


It would be best to create the zip archive in a deterministic way; it's probably easiest to fold this into archive_deterministically or make a similar helper method. See https://reproducible-builds.org/docs/archives/ for a list of major considerations.

That method also has some other important features, like using with statements to close files and automatically un-cd, and atomic renaming of the output to prevent half-finished archives from being uploaded.

aneeshusa · 2017-04-23T15:58:55Z

python/servo/build_commands.py

+                        vs_dll_dir = redist1
+                    elif os.path.isdir(redist2):
+                        vs_dll_dir = redist2
+                if vs_version == "15.0" or is_vs14:


I'm not sure, but I think if vs_dll_dir: may have better semantics here.
Also, if these deps are required, it would be nice to have some error handling in case we can't find the vs_dll_dir.

aneeshusa · 2017-04-23T15:58:55Z

python/servo/build_commands.py

+                        vs_dll_dir,
+                        path.join(os.environ["WindowsSdkDir"], "Redist", "ucrt", "DLLs", vs_platform),
+                    ]
+                    import stat


Move to the top level.

aneeshusa · 2017-04-23T15:58:55Z

python/servo/build_commands.py

+                    ]
+                    import stat
+                    for msvc_dll in msvc_deps:
+                        for dll_dir in dll_dirs:


This seems to copy each dll from two different locations to the same place in the servo_dir_dll. Is the intent to instead do one copy of dll, from the first location it is found?

I added break in loop, but doesn't make a difference, because in Windows Sdk directory are only api-ms-*.dll files

aneeshusa · 2017-04-23T15:58:55Z

python/servo/build_commands.py

+                            servo_dir_dll = path.join(servo_exe_dir, msvc_dll)
+                            if os.path.isfile(dll):
+                                if os.path.isfile(servo_dir_dll):
+                                    # avoid permission denied error when overwrite dll in servo build directory


Why would we overwrite DLLs? If we already have the DLL, why copy (presumably) the same DLL over top?

This will make sure that DLLs are always up-to-date.

mbrubeck · 2017-09-21T17:58:51Z

@bors-servo clean retry

bors-servo · 2017-09-21T17:58:56Z

⌛ Testing commit 67b1a4e with merge 9dc25a5...


        Auto merge of #16445 - UK992:mach-package, r=larsbergstrom

Windows: Add missing MSVC dependencies - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #16422  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16445)

bors-servo · 2017-09-21T18:20:26Z

💔 Test failed - windows-msvc-dev


        Windows: Add missing MSVC dependencies


        Package: Update zip file creation

Attempt to fix, (dot) folder in zip file


        fixup! Windows: Add missing MSVC dependencies


        test

UK992 · 2017-09-24T17:52:47Z

I made some changes in code for vs 2017.
Please run try build to see why api-ms-win-crt-runtime-l1-1-0.dll is missing. thanks

aneeshusa · 2017-09-24T19:31:16Z

@bors-servo try

bors-servo · 2017-09-24T19:31:21Z

⌛ Trying commit fce7f85 with merge 06d147d...


        Auto merge of #16445 - UK992:mach-package, r=<try>

Windows: Add missing MSVC dependencies - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #16422  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16445)

bors-servo · 2017-09-24T19:52:39Z

💔 Test failed - windows-msvc-dev

UK992 · 2017-09-24T20:33:37Z

Looks like buildbot machines need to update Windows SDK.

bors-servo · 2018-02-13T14:54:58Z

☔ The latest upstream changes (presumably #19975) made this pull request unmergeable. Please resolve the merge conflicts.


        Auto merge of #21968 - UK992:mach-package, r=jdm

Windows: Add missing dependencies Rebased #16445 and updated with Gstreamer DLLs. About msi installer, there is also included gstreamer installer, should be removed and replaced by needed gstreamer DLLs or keep it at is it?  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21968)


        Auto merge of #21968 - UK992:mach-package, r=jdm

Windows: Add missing dependencies Rebased #16445 and updated with Gstreamer DLLs. About msi installer, there is also included gstreamer installer, should be removed and replaced by needed gstreamer DLLs or keep it at is it? Fixes #16422.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21968)


        Auto merge of #21968 - UK992:mach-package, r=<try>

Windows: Add missing dependencies Rebased #16445 and updated with Gstreamer DLLs. About msi installer, there is also included gstreamer installer, should be removed and replaced by needed gstreamer DLLs or keep it at is it? Fixes #16422.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21968)


        Auto merge of #21968 - UK992:mach-package, r=<try>

Windows: Add missing dependencies Rebased #16445 and updated with Gstreamer DLLs. About msi installer, there is also included gstreamer installer, should be removed and replaced by needed gstreamer DLLs or keep it at is it? Fixes #16422.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21968)


        Auto merge of #21968 - UK992:mach-package, r=<try>

Windows: Add missing dependencies Rebased #16445 and updated with Gstreamer DLLs. About msi installer, there is also included gstreamer installer, should be removed and replaced by needed gstreamer DLLs or keep it at is it? Fixes #16422.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21968)


        Auto merge of #21968 - UK992:mach-package, r=jdm

Windows: Add missing dependencies Rebased #16445 and updated with Gstreamer DLLs. About msi installer, there is also included gstreamer installer, should be removed and replaced by needed gstreamer DLLs or keep it at is it? Fixes #16422.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21968)

highfive assigned KiChjang Apr 13, 2017

highfive added the S-awaiting-review label Apr 13, 2017

aneeshusa requested changes Apr 14, 2017

View changes

aneeshusa assigned aneeshusa and unassigned KiChjang Apr 14, 2017

UK992 force-pushed the UK992:mach-package branch from 0ac6181 to 71614d3 Apr 14, 2017

aneeshusa requested changes Apr 14, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Apr 14, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 14, 2017

aneeshusa added S-awaiting-answer S-needs-squash and removed S-awaiting-review labels Apr 15, 2017

highfive added the S-awaiting-review label Apr 15, 2017

UK992 force-pushed the UK992:mach-package branch from d9e42b9 to a186ee0 Apr 15, 2017

highfive added the S-needs-rebase label Apr 17, 2017

UK992 force-pushed the UK992:mach-package branch from a186ee0 to 72459b0 Apr 17, 2017

UK992 force-pushed the UK992:mach-package branch from 78f9c24 to b353f9e Apr 19, 2017

jdm removed S-needs-rebase S-needs-squash labels Apr 20, 2017

aneeshusa requested changes Apr 23, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-answer labels Apr 23, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 21, 2017

UK992 added 4 commits Apr 13, 2017

Windows: Add missing MSVC dependencies

2bbee1a

Package: Update zip file creation

756c728

Attempt to fix, (dot) folder in zip file

fixup! Windows: Add missing MSVC dependencies

c001a97

test

Loading status checks…

fce7f85

UK992 force-pushed the UK992:mach-package branch from 67b1a4e to fce7f85 Sep 24, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Sep 24, 2017

highfive added the S-tests-failed label Sep 24, 2017

highfive added the S-needs-rebase label Feb 13, 2018

UK992 closed this Oct 16, 2018

UK992 mentioned this pull request Oct 16, 2018

Windows: Add missing dependencies #21968

Merged

servo / servo

Windows: Add missing MSVC dependencies #16445

Windows: Add missing MSVC dependencies #16445

UK992 commented Apr 13, 2017 •

edited

highfive commented Apr 13, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Apr 15, 2017

UK992 commented Apr 15, 2017 •

edited

bors-servo commented Apr 17, 2017

jdm commented Apr 17, 2017

larsbergstrom commented Apr 23, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mbrubeck commented Sep 21, 2017

bors-servo commented Sep 21, 2017

bors-servo commented Sep 21, 2017

UK992 commented Sep 24, 2017

aneeshusa commented Sep 24, 2017

bors-servo commented Sep 24, 2017

bors-servo commented Sep 24, 2017

UK992 commented Sep 24, 2017

bors-servo commented Feb 13, 2018

servo / servo

Join GitHub today

Windows: Add missing MSVC dependencies #16445

Windows: Add missing MSVC dependencies #16445

Conversation

UK992 commented Apr 13, 2017 • edited

highfive commented Apr 13, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Apr 15, 2017

UK992 commented Apr 15, 2017 • edited

bors-servo commented Apr 17, 2017

jdm commented Apr 17, 2017

larsbergstrom commented Apr 23, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mbrubeck commented Sep 21, 2017

bors-servo commented Sep 21, 2017

bors-servo commented Sep 21, 2017

UK992 commented Sep 24, 2017

aneeshusa commented Sep 24, 2017

bors-servo commented Sep 24, 2017

bors-servo commented Sep 24, 2017

UK992 commented Sep 24, 2017

bors-servo commented Feb 13, 2018

UK992 commented Apr 13, 2017 •

edited

UK992 commented Apr 15, 2017 •

edited