Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix checkouts of standing data for tests on Windows, enable vcpkg on macOS and Linux #420

Merged
merged 7 commits into from
Aug 18, 2019

Conversation

mcgoo
Copy link
Contributor

@mcgoo mcgoo commented Aug 1, 2019

@crlf0710 added the ability to build using msvc with libraries from vcpkg.

I'm not sure if it is useful to you, but vcpkg-rs 0.2.7 adds support for Linux and macOS so I enabled it for them also. (This does allow using the exact same versions of libraries across all platforms which may be of interest.) Windows (msvc) works for static and dynamic builds, and Linux and macOS work for static builds. Almost everything builds and passes tests, but on Windows, one of the doctests still fails the first time it is run.

This fixes the issue with the line endings in the test data getting changed to CRLF when it is checked out on Windows, by adding a .gitattributes file. This does not update an existing checkout so it is probably necessary to delete the tests/ directory and check it out again to get the unchanged versions. (This is particularly painful if you clone the repo and .gitattributes is not on the master branch yet - git will not change the line endings as you change branches.)

For a Windows static build I used:

.\vcpkg install --triplet x64-windows-static fontconfig freetype harfbuzz[icu,graphite2]
set VCPKG_ROOT=<wherever>
set TECTONIC_VCPKG=1
set RUSTFLAGS=-Ctarget-feature=+crt-static
cargo test

For a Linux static build I used:

./vcpkg install fontconfig freetype harfbuzz\[icu,graphite2\]
set VCPKG_ROOT=<wherever>
set TECTONIC_VCPKG=1
cargo test

For a macOS static build I used:

./vcpkg install freetype harfbuzz\[icu,graphite2\]
set VCPKG_ROOT=<wherever>
set TECTONIC_VCPKG=1
cargo test

For a Windows dynamic build I used:

.\vcpkg install --triplet x64-windows fontconfig freetype harfbuzz[icu,graphite2]
set VCPKG_ROOT=<wherever>
set TECTONIC_VCPKG=1
set VCPKGRS_DYNAMIC=1
cargo test

@pkgw pkgw self-assigned this Aug 1, 2019
@mcgoo
Copy link
Contributor Author

mcgoo commented Aug 1, 2019

By the way, setting RUST_TEST_THREADS=1 would hide the file locking issue on tests\0000000000000000000000000000000000000000000000000000000000000000-latex-28.fmt which is what is causing the test failure.

@ghost ghost mentioned this pull request Aug 5, 2019
@ghost
Copy link

ghost commented Aug 6, 2019

This looks quite interesting; I tried to build a static version of tectonic on macOS but ran into some issues. Summarizing my experience below for future travelers and discussion.
It seems there are a couple ways to build static binaries:

I'm familiar with the first two options and so will take one of those approaches to get a static build working. I do not have pkg-config installed globally so I think it tripped up build.rs . As an aside, rust-lang/pkg-config-rs#56 confused me a bit while debugging so it would be nice to fix that.

If we do end up using vcpkg we should make users aware that it collects telemetry data by default.

@pkgw
Copy link
Collaborator

pkgw commented Aug 6, 2019

I would be interested in how the static build on macOS works. For Linux, the problem is that the standard Tectonic build uses procedural macros in the compiler, which requires that the compiler support dynamic linking. A lot of people in the Rust community have had problems of this general nature. My solution, which I think is the best available, is to treat the compilation as a cross-compilation, from a dynamic Linux platform to a static Linux platform. I think there might be some hacky magic going on in the toolchain to make that work, but work it does.

For macOS, there must be system libraries for font-finding that are guaranteed to be installed — does a static build miss out on those?

@pkgw
Copy link
Collaborator

pkgw commented Aug 6, 2019

Also, please note that CI has been stuck for a bit, and I recently merged some changes to fix things up. I highly recommend that you rebase on master once I have cleared the backlog of Dependabot PRs.

@mcgoo
Copy link
Contributor Author

mcgoo commented Aug 7, 2019

I think I might have over-sold the static. On Linux and macOS, vcpkg will compile everything to static libs, so any dependencies coming in from there are statically linked. Here's what the binaries look like:

$ otool -L target/release/tectonic
target/release/tectonic:
	/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1659.10.0)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1659.10.0)
	/System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics (compatibility version 64.0.0, current version 1336.0.0)
	/System/Library/Frameworks/CoreText.framework/Versions/A/CoreText (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit (compatibility version 45.0.0, current version 1865.10.102)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 800.6.0)
	/System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 59286.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0)
	/usr/lib/libresolv.9.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
$ ldd target/debug/tectonic
        linux-vdso.so.1 (0x00007ffc07194000)
        libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f8f631d6000)
        libssl.so.1.1 => /usr/lib/x86_64-linux-gnu/libssl.so.1.1 (0x00007f8f62f49000)
        libcrypto.so.1.1 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f8f62a7e000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f8f6287a000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f8f62672000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f8f62453000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f8f6223b000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f8f61e4a000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f8f66a55000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f8f61aac000)

@codecov
Copy link

codecov bot commented Aug 7, 2019

Codecov Report

Merging #420 into master will increase coverage by 0.5%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #420     +/-   ##
=========================================
+ Coverage    43.7%   44.21%   +0.5%     
=========================================
  Files         136      136             
  Lines       59536    60053    +517     
=========================================
+ Hits        26019    26550    +531     
+ Misses      33517    33503     -14
Impacted Files Coverage Δ
tectonic/xetex-XeTeXLayoutInterface.cpp 27.92% <ø> (ø) ⬆️
tectonic/xetex-XeTeXFontMgr.cpp 27.24% <ø> (ø) ⬆️
tectonic/dpx-pngimage.c 48.45% <0%> (-11.64%) ⬇️
src/config.rs 6.32% <0%> (+0.07%) ⬆️
tectonic/xetex-ini.c 87.24% <0%> (+0.25%) ⬆️
tests/tex-outputs.rs 96.96% <0%> (+0.35%) ⬆️
tectonic/bibtex.c 51.32% <0%> (+7.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85f4b18...10bee2e. Read the comment docs.

@pkgw
Copy link
Collaborator

pkgw commented Aug 8, 2019

I made a test PR (#426) that seems to show that indeed, setting RUST_TEST_THREADS=1 gets the full CI test suite to pass. I would like to understand what is going on in the current situation, but I don't have the bandwidth to work on that right now and I'd rather just get this merged, so I'm OK with just working around it.

The other thing is that I'd like to rearrange how the build.rs works a bit. I think it would be best to have the environment variable be something like TECTONIC_NATIVE_DEP_FINDER that defaults to pkg-config but can be overridden to vcpkg and, potentially in the future, other values. I can make this change myself, @mcgoo, if you give me permission to push changes to your branch (instructions).

@mcgoo
Copy link
Contributor Author

mcgoo commented Aug 8, 2019

Looks like "allow edits from maintainers" is set on this PR, #420 so you should be good to go I think.

@pkgw
Copy link
Collaborator

pkgw commented Aug 15, 2019

@mcgoo Hmm, for some reason I'm still getting "permission denied" — oh, maybe it allows pushes but not force-pushes? I wouldn't be surprised if that was the case. #426 has my version rebased onto latest master, but still not including the different approach to the build.rs configuration that I'd like to try.

@mcgoo
Copy link
Contributor Author

mcgoo commented Aug 16, 2019

I made you a collaborator on mcgoo/tectonic to see if that does it. Have you done the modifications to build.rs? If you have not, I can take a look at that and rebasing again?

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@a798723). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #420   +/-   ##
========================================
  Coverage          ?   43.7%           
========================================
  Files             ?     136           
  Lines             ?   59536           
  Branches          ?       0           
========================================
  Hits              ?   26019           
  Misses            ?   33517           
  Partials          ?       0
Impacted Files Coverage Δ
tectonic/xetex-XeTeXLayoutInterface.cpp 27.92% <ø> (ø)
tectonic/xetex-XeTeXFontMgr.cpp 27.24% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a798723...396d5dd. Read the comment docs.

@pkgw
Copy link
Collaborator

pkgw commented Aug 18, 2019

There seems to be some issue with CI statuses updating right now, but I've structured things the way I was thinking and everything that matters is green. Merging!

@pkgw pkgw merged commit 7315ba1 into tectonic-typesetting:master Aug 18, 2019
@ghost ghost mentioned this pull request Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants