Switch to v5 UUIDs as profile GUIDs for the default profiles #913

DHowett-MSFT · 2019-05-21T04:19:57Z

Summary of the Pull Request

This pull request switches the GUIDs for default profiles from being randomly generated to being version 5 UUIDs. More info in #870.

PR Checklist

Closes Feature Request: stable UUIDs for default profiles #870
CLA signed
Tests added/passed
Requires documentation to be updated (adding profiles.json documentation #883)
I've discussed this with core contributors already.

Detailed Description of the Pull Request / Additional comments

This pull request has a number of changes that seem ancillary, but they're general goodness. Let me explain:

I've added a whole new Types test library with only two tests in
Since UUIDv5 generation requires SHA1, we needed to take a dependency on bcrypt
I honestly don't think we should have to link bcrypt in conhost, but LTO should take care of that
- I considered adding a new Terminal-specific Utils/Types library, but that seemed like a waste
The best way to link bcrypt turned out to be in line with a discussion @miniksa and I had, where we decided we both love APISets and think that the console should link against them exclusively... so I've added onecore_apiset.lib to the front of the link line, where it will deflect the linker away from most of the other libs automagically.

StartGroup: UuidTests::TestV5UuidU8String
Verify: AreEqual(uuidExpected, uuidActual)
EndGroup: UuidTests::TestV5UuidU8String [Passed]

StartGroup: UuidTests::TestV5UuidU16String
Verify: AreEqual(uuidExpected, uuidActual)
EndGroup: UuidTests::TestV5UuidU16String [Passed]

Closes #870.

src/types/utils.cpp

DHowett-MSFT · 2019-05-21T05:35:01Z

@miniksa Bad news! The exe.or.dll.props props are included before VS's default library dependencies, so unless we restructure the include ordering for these things they end up searching the libraries in the wrong order.

if we fix that:

> depends openconsole.exe
=== C:\Users\Dustin\Projects\thirdparty\OpenConsole-master\bin\x64\Debug\openconsole.exe ===

  Dependencies:
    api-ms-win-core-com-l1-1-0
    api-ms-win-core-debug-l1-1-0
    api-ms-win-core-errorhandling-l1-1-0
    api-ms-win-core-file-l1-1-0
    api-ms-win-core-handle-l1-1-0
    api-ms-win-core-heap-l1-1-0
    api-ms-win-core-heap-l2-1-0
    api-ms-win-core-interlocked-l1-1-0
    api-ms-win-core-io-l1-1-0
    api-ms-win-core-largeinteger-l1-1-0
    api-ms-win-core-libraryloader-l1-2-0
    api-ms-win-core-libraryloader-l1-2-1
    api-ms-win-core-localization-l1-2-0
    api-ms-win-core-memory-l1-1-0
    api-ms-win-core-path-l1-1-0
    api-ms-win-core-processenvironment-l1-1-0
    api-ms-win-core-processthreads-l1-1-0
    api-ms-win-core-processthreads-l1-1-1
    api-ms-win-core-profile-l1-1-0
    api-ms-win-core-psapi-l1-1-0
    api-ms-win-core-registry-l1-1-0
    api-ms-win-core-registry-l1-1-1
    api-ms-win-core-registry-l2-1-0
    api-ms-win-core-rtlsupport-l1-1-0
    api-ms-win-core-sidebyside-l1-1-0
    api-ms-win-core-string-l1-1-0
    api-ms-win-core-synch-l1-1-0
    api-ms-win-core-synch-l1-2-0
    api-ms-win-core-sysinfo-l1-1-0
    api-ms-win-core-threadpool-legacy-l1-1-0
    api-ms-win-core-util-l1-1-0
    api-ms-win-eventing-provider-l1-1-0
    api-ms-win-shcore-obsolete-l1-1-0
    api-ms-win-shcore-scaling-l1-1-1
    bcrypt.dll
    d2d1.dll
    d3d11.dll
    dwmapi.dll
    dwrite.dll
    dxgi.dll
    gdi32.dll
    msvcp140d.dll
    ntdll.dll
    oleaut32.dll
    propsys.dll
    shell32.dll
    shlwapi.dll
    ucrtbased.dll
    uiautomationcore.dll
    user32.dll
    uxtheme.dll
    vcruntime140d.dll
    winmm.dll

src/types/ut/UuidTests.cpp

src/types/utils.cpp

zadjii-msft

Other than the tests thing, looks good

src/cascadia/TerminalApp/CascadiaSettings.cpp

OpenConsole.sln

DHowett-MSFT · 2019-05-21T15:01:06Z

@zadjii-msft you called types “dependency-free”, so I should feel less comfortable adding a dependency on bcrypt.lib from it. Even though it’s in OneCore... hrm

zadjii-msft · 2019-05-21T15:26:55Z

@DHowett-MSFT when I say "dependency-free", I guess I meant more that it didn't have any other dependencies w/in our codebase. It was a bottom(?)-clean lib that could be used by any other lib without introducing a bunch of other dependencies. This is contrasted with the server lib, which is also fundamentally dependent upon the host lib

miniksa · 2019-05-21T15:55:57Z

@miniksa Michael Niksa FTE Bad news! The exe.or.dll.props props are included before VS's default library dependencies, so unless we restructure the include ordering for these things they end up searching the libraries in the wrong order.

if we fix that:

> depends openconsole.exe
=== C:\Users\Dustin\Projects\thirdparty\OpenConsole-master\bin\x64\Debug\openconsole.exe ===

  Dependencies:
    api-ms-win-core-com-l1-1-0
    api-ms-win-core-debug-l1-1-0
    api-ms-win-core-errorhandling-l1-1-0
    api-ms-win-core-file-l1-1-0
    api-ms-win-core-handle-l1-1-0
    api-ms-win-core-heap-l1-1-0
    api-ms-win-core-heap-l2-1-0
    api-ms-win-core-interlocked-l1-1-0
    api-ms-win-core-io-l1-1-0
    api-ms-win-core-largeinteger-l1-1-0
    api-ms-win-core-libraryloader-l1-2-0
    api-ms-win-core-libraryloader-l1-2-1
    api-ms-win-core-localization-l1-2-0
    api-ms-win-core-memory-l1-1-0
    api-ms-win-core-path-l1-1-0
    api-ms-win-core-processenvironment-l1-1-0
    api-ms-win-core-processthreads-l1-1-0
    api-ms-win-core-processthreads-l1-1-1
    api-ms-win-core-profile-l1-1-0
    api-ms-win-core-psapi-l1-1-0
    api-ms-win-core-registry-l1-1-0
    api-ms-win-core-registry-l1-1-1
    api-ms-win-core-registry-l2-1-0
    api-ms-win-core-rtlsupport-l1-1-0
    api-ms-win-core-sidebyside-l1-1-0
    api-ms-win-core-string-l1-1-0
    api-ms-win-core-synch-l1-1-0
    api-ms-win-core-synch-l1-2-0
    api-ms-win-core-sysinfo-l1-1-0
    api-ms-win-core-threadpool-legacy-l1-1-0
    api-ms-win-core-util-l1-1-0
    api-ms-win-eventing-provider-l1-1-0
    api-ms-win-shcore-obsolete-l1-1-0
    api-ms-win-shcore-scaling-l1-1-1
    bcrypt.dll
    d2d1.dll
    d3d11.dll
    dwmapi.dll
    dwrite.dll
    dxgi.dll
    gdi32.dll
    msvcp140d.dll
    ntdll.dll
    oleaut32.dll
    propsys.dll
    shell32.dll
    shlwapi.dll
    ucrtbased.dll
    uiautomationcore.dll
    user32.dll
    uxtheme.dll
    vcruntime140d.dll
    winmm.dll

You can order them however is necessary to get linkage to work correctly. The only reason I might have said something like "the order is important here, watch yourself" in a comment near those is for the common-editing case (to scare folks away). Not for this case where we're actually trying to change the whole linking world.

miniksa · 2019-05-21T15:57:50Z

@zadjii-msft Mike Griese FTE you called types “dependency-free”, so I should feel less comfortable adding a dependency on bcrypt.lib from it. Even though it’s in OneCore... hrm

@DHowett-MSFT Dustin Howett FTE when I say "dependency-free", I guess I meant more that it didn't have any other dependencies w/in our codebase. It was a bottom(?)-clean lib that could be used by any other lib without introducing a bunch of other dependencies. This is contrasted with the server lib, which is also fundamentally dependent upon the host lib

I am not concerned about external dependencies. Technically the whole types library has a CRT and STL dependency. It's more that our internal project-to-project layering should strive to be less garbage over time (per where @zadjii-msft was going with his comment.)

OpenConsole.sln

src/cascadia/TerminalApp/CascadiaSettings.cpp

src/cascadia/TerminalApp/Profile.cpp

src/common.build.exe.or.dll.props

src/types/ut/Types.Unit.Test.vcxproj

src/types/utils.cpp

src/cascadia/TerminalApp/CascadiaSettings.cpp

that was harrowing

src/types/ut_types/sources

src/types/dirs

DHowett-MSFT · 2019-05-21T19:59:17Z

Followups filed!

DHowett added 4 commits May 16, 2019 18:09

Generate UUIDv5s for default profiles

226f6bb

Closes #870.

add a comment about using memcpy

fbe1647

Clean up

300d3fa

move to types, add tests, finish almost everything

242d3ae

DHowett-MSFT added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels May 21, 2019

DHowett-MSFT added this to the Terminal v0.2 milestone May 21, 2019

DHowett-MSFT commented May 21, 2019

View reviewed changes

src/types/utils.cpp Show resolved Hide resolved

DHowett-MSFT requested review from zadjii-msft and miniksa May 21, 2019 04:23

vs is good at what it does

066edfa

DHowett-MSFT mentioned this pull request May 21, 2019

set identifying environment variable for new connections #897

Merged

5 tasks

Name the test dll something reasonable

395a8c4

flug reviewed May 21, 2019

View reviewed changes

src/types/ut/UuidTests.cpp Outdated Show resolved Hide resolved

src/types/ut/UuidTests.cpp Outdated Show resolved Hide resolved

src/types/utils.cpp Outdated Show resolved Hide resolved

src/types/utils.cpp Outdated Show resolved Hide resolved

zadjii-msft approved these changes May 21, 2019

View reviewed changes

src/cascadia/TerminalApp/CascadiaSettings.cpp Show resolved Hide resolved

OpenConsole.sln Outdated Show resolved Hide resolved

miniksa requested changes May 21, 2019

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 21, 2019

miniksa mentioned this pull request May 21, 2019

Check status of TextBuffer Unit Tests in CI #854

Closed

binarycrusader reviewed May 21, 2019

View reviewed changes

src/types/utils.cpp Show resolved Hide resolved

binarycrusader reviewed May 21, 2019

View reviewed changes

src/types/utils.cpp Outdated Show resolved Hide resolved

Address CR feedback

74e7eeb

binarycrusader reviewed May 21, 2019

View reviewed changes

src/cascadia/TerminalApp/CascadiaSettings.cpp Show resolved Hide resolved

build it for Windows too

89f79c7

that was harrowing

miniksa reviewed May 21, 2019

View reviewed changes

src/types/ut_types/sources Show resolved Hide resolved

miniksa reviewed May 21, 2019

View reviewed changes

src/types/dirs Outdated Show resolved Hide resolved

more test things, cr

e8cef93

miniksa approved these changes May 21, 2019

View reviewed changes

This was referenced May 21, 2019

Code Health: Follow ES.9: Avoid ALL_CAPS names per ISO C++ guidelines #921

Open

Adjust the link line for all exe/dlls to prefer onecore_apiset #922

Closed

DHowett-MSFT merged commit 8da6737 into master May 21, 2019

DHowett-MSFT deleted the dev/duhowett/uuidv5 branch May 21, 2019 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to v5 UUIDs as profile GUIDs for the default profiles #913

Switch to v5 UUIDs as profile GUIDs for the default profiles #913

DHowett-MSFT commented May 21, 2019 •

edited

Loading

DHowett-MSFT commented May 21, 2019

zadjii-msft left a comment

DHowett-MSFT commented May 21, 2019

zadjii-msft commented May 21, 2019

miniksa commented May 21, 2019

miniksa commented May 21, 2019

DHowett-MSFT commented May 21, 2019

Switch to v5 UUIDs as profile GUIDs for the default profiles #913

Switch to v5 UUIDs as profile GUIDs for the default profiles #913

Conversation

DHowett-MSFT commented May 21, 2019 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

DHowett-MSFT commented May 21, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented May 21, 2019

zadjii-msft commented May 21, 2019

miniksa commented May 21, 2019

miniksa commented May 21, 2019

DHowett-MSFT commented May 21, 2019

DHowett-MSFT commented May 21, 2019 •

edited

Loading