Create vcxproj for yaml-cpp and add manifest helper lib with tests #14

yao-msft · 2019-12-17T19:40:57Z

This makes yaml-cpp code compiled as part of the AppInstallerClient solution.

JohnMcPMS · 2019-12-17T20:35:46Z

src/YamlCppLib/readme.txt

@@ -0,0 +1,12 @@
+Do not change code under yaml-cpp directory. yaml-cpp directory contains yaml-cpp source code from https://github.com/jbeder/yaml-cpp.


Do [](start = 0, length = 2)

Please rename to README.md and use markdown #Closed

JohnMcPMS · 2019-12-17T20:39:36Z

src/YamlCppLib/YamlCppLib.vcxproj

+    <PlatformToolset>v140</PlatformToolset>
+    <PlatformToolset Condition="'$(VisualStudioVersion)' == '15.0'">v141</PlatformToolset>
+    <PlatformToolset Condition="'$(VisualStudioVersion)' == '16.0'">v142</PlatformToolset>
+    <CharacterSet>Unicode</CharacterSet>


CharacterSet [](start = 5, length = 12)

Please set the build to use C++17. If that does not work, we will need to investigate link compatibility. #Closed

JohnMcPMS · 2019-12-17T20:40:15Z

You will need to update cgmanifest in the root to include this new dependency. #Closed

JohnMcPMS · 2019-12-17T20:40:58Z

Please add at least a test binary that invokes this code to ensure that it is building and working properly. #Closed

JohnMcPMS

🕐

yao-msft · 2019-12-17T23:06:05Z

Chatted I'll check in this together with manifesthelper lib

In reply to: 566740080 [](ancestors = 566740080)

yao-msft · 2019-12-19T19:30:10Z

platform: 'x86'

John, is this a mistake we only build x86 here?
#WontFix

Refers to: azure-pipelines.yml:29 in a314fa1. [](commit_id = a314fa1, deletion_comment = False)

JohnMcPMS · 2019-12-19T21:25:48Z

platform: 'x86'

The packages get built in all of those from 'buildPlatform', but anything outside of those will be built x86. I haven't invested much into getting this exactly right atm.

In reply to: 567629920 [](ancestors = 567629920)

Refers to: azure-pipelines.yml:29 in a314fa1. [](commit_id = a314fa1, deletion_comment = False)

JohnMcPMS · 2019-12-19T21:47:10Z

src/PackageManifestHelper/Manifest.cpp

+            YAML::Node installerNode = installersNode[i];
+            ManifestInstaller installer;
+            installer.PopulateInstallerFields(installerNode);
+            this->Installers.push_back(installer);


push_back [](start = 29, length = 9)

Use 'emplace_back(std::move(installer))' so that we don't have to make copies of all the strings #Resolved

JohnMcPMS · 2019-12-19T21:51:37Z

src/PackageManifestHelper/PackageManifestHelper.cpp

+#include "manifest.h"
+
+// TODO: This is an example of a library function
+void fnPackageManifestHelper()


fnPackageManifestHelper [](start = 5, length = 23)

What is this for? #Resolved

Accidentally checked in, I thought I had removed it. I'll remove in the next push

In reply to: 360134880 [](ancestors = 360134880)

JohnMcPMS · 2019-12-19T21:52:25Z

src/PackageManifestHelper/Manifest.cpp

+
+#include "pch.h"
+#include "framework.h"
+#include "yaml-cpp\yaml.h"


"yaml-cpp\yaml.h" [](start = 9, length = 17)

This should go into the pch #Resolved

JohnMcPMS · 2019-12-19T21:53:55Z

src/PackageManifestHelperTests/PackageManifestHelper.cpp

+{
+    Manifest manifest = Manifest::CreatePackageManifest("GoodManifest.yml");
+
+    REQUIRE(manifest.Id.compare("microsoft.msixsdk") == 0);


compare [](start = 24, length = 7)

Do no use compare for this, simply do 'manifest.Id == "microsoft.msixsdk"' #Resolved

JohnMcPMS · 2019-12-19T21:54:19Z

src/PackageManifestHelperTests/PackageManifestHelper.cpp

+
+using namespace AppInstaller::Package::Manifest;
+
+TEST_CASE("Read good package manifest and verify contents", "[PackageManifestHelper]")


Read good package manifest and verify contents [](start = 11, length = 46)

This is the test case name, lets not put spaces in it. #Resolved

JohnMcPMS · 2019-12-19T21:54:37Z

src/PackageManifestHelperTests/PackageManifestHelper.cpp

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+#include "pch.h"
+#include "yaml-cpp\yaml.h"


Put in the pch #Resolved

ranm-msft · 2019-12-19T22:04:42Z

src/PackageManifestHelper/Manifest.h

+        std::string Version;
+
+        // Name subject to change
+        std::string ShortId;


ShortId [](start = 20, length = 7)

please add explanation + example #WontFix

This isn't the location for that kind of documentation.

In reply to: 360142170 [](ancestors = 360142170)

JohnMcPMS · 2019-12-19T22:06:38Z

src/PackageManifestHelperTests/main.cpp

+using namespace winrt;
+using namespace Windows::Foundation;
+
+int main(int argc, char** argv)


main [](start = 4, length = 4)

Sooooo, I'm now thinking maybe we should collapse down to a single test binary... I'm not sure its worth it to have separate ones. #Resolved

Do you want me to do it now or later

In reply to: 360143037 [](ancestors = 360143037)

Now is fine. If you want to just put the tests and TestData into the existing one, I can rename it in my PR once I merger your change in.

In reply to: 360147613 [](ancestors = 360147613,360143037)

ranm-msft · 2019-12-19T23:00:01Z

src/PackageManifestHelper/Manifest.cpp

+    }
+
+    Manifest Manifest::CreatePackageManifest(const std::string& inputFile)
+    {


{ [](start = 4, length = 1)

Add logging throughout the project ex. file path #Pending

Currently only tracelogging is enabled. we'll add a logger wrapper and log in a following PR

In reply to: 360160037 [](ancestors = 360160037)

JohnMcPMS · 2019-12-19T23:29:40Z

src/PackageManifestHelper/Manifest.h

+#include "ManifestInstaller.h"
+#include "ManifestLocalization.h"
+
+namespace AppInstaller::Package::Manifest


Package::Manifest [](start = 24, length = 17)

Could we just name this part YAML #Resolved

Chatted offline and we'll use AppInstaller::Manifest

In reply to: 360168250 [](ancestors = 360168250)

JohnMcPMS

yao-msft added 3 commits December 16, 2019 17:27

Create VS proj file for yaml-cpp code

3905d30

proj file

cb7c48f

readme

5279cec

yao-msft requested a review from a team as a code owner December 17, 2019 19:40

JohnMcPMS reviewed Dec 17, 2019

View reviewed changes

JohnMcPMS requested changes Dec 17, 2019

View reviewed changes

PR comments

ca842ff

yao-msft force-pushed the vcproj branch from ddc69cb to ca842ff Compare December 17, 2019 23:03

yao-msft added 3 commits December 19, 2019 11:21

Helperlib

5320600

test

0487731

fix proj settings

a314fa1

yao-msft changed the title ~~Create vcxproj for yaml-cpp~~ Create vcxproj for yaml-cpp and add manifest helper lib with tests Dec 19, 2019

fix test script

bf924f2

JohnMcPMS reviewed Dec 19, 2019

View reviewed changes

ranm-msft reviewed Dec 19, 2019

View reviewed changes

JohnMcPMS reviewed Dec 19, 2019

View reviewed changes

PR comments

ab9ca6f

ranm-msft reviewed Dec 19, 2019

View reviewed changes

JohnMcPMS reviewed Dec 19, 2019

View reviewed changes

yao-msft added 2 commits December 19, 2019 15:53

Merge manifest helper tests

d09bc13

More PR comments

cd53091

JohnMcPMS approved these changes Dec 20, 2019

View reviewed changes

JohnMcPMS merged commit 595bb47 into microsoft:master Dec 20, 2019

yao-msft deleted the vcproj branch December 21, 2019 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create vcxproj for yaml-cpp and add manifest helper lib with tests #14

Create vcxproj for yaml-cpp and add manifest helper lib with tests #14

yao-msft commented Dec 17, 2019

JohnMcPMS Dec 17, 2019 •

edited

JohnMcPMS Dec 17, 2019 •

edited

JohnMcPMS commented Dec 17, 2019 •

edited

JohnMcPMS commented Dec 17, 2019 •

edited

JohnMcPMS left a comment

yao-msft commented Dec 17, 2019

yao-msft commented Dec 19, 2019 •

edited by JohnMcPMS

JohnMcPMS commented Dec 19, 2019

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

yao-msft Dec 19, 2019

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

ranm-msft Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

yao-msft Dec 19, 2019

JohnMcPMS Dec 19, 2019

ranm-msft Dec 19, 2019 •

edited by yao-msft

yao-msft Dec 20, 2019

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

yao-msft Dec 20, 2019

JohnMcPMS left a comment

		@@ -0,0 +1,12 @@
		Do not change code under yaml-cpp directory. yaml-cpp directory contains yaml-cpp source code from https://github.com/jbeder/yaml-cpp.


		using namespace AppInstaller::Package::Manifest;

		TEST_CASE("Read good package manifest and verify contents", "[PackageManifestHelper]")

Create vcxproj for yaml-cpp and add manifest helper lib with tests #14

Create vcxproj for yaml-cpp and add manifest helper lib with tests #14

Conversation

yao-msft commented Dec 17, 2019

JohnMcPMS Dec 17, 2019 • edited

Choose a reason for hiding this comment

JohnMcPMS Dec 17, 2019 • edited

Choose a reason for hiding this comment

JohnMcPMS commented Dec 17, 2019 • edited

JohnMcPMS commented Dec 17, 2019 • edited

JohnMcPMS left a comment

Choose a reason for hiding this comment

yao-msft commented Dec 17, 2019

yao-msft commented Dec 19, 2019 • edited by JohnMcPMS

JohnMcPMS commented Dec 19, 2019

JohnMcPMS Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

JohnMcPMS Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

yao-msft Dec 19, 2019

Choose a reason for hiding this comment

JohnMcPMS Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

JohnMcPMS Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

JohnMcPMS Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

JohnMcPMS Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

ranm-msft Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

JohnMcPMS Dec 19, 2019

Choose a reason for hiding this comment

JohnMcPMS Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

yao-msft Dec 19, 2019

Choose a reason for hiding this comment

JohnMcPMS Dec 19, 2019

Choose a reason for hiding this comment

ranm-msft Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

yao-msft Dec 20, 2019

Choose a reason for hiding this comment

JohnMcPMS Dec 19, 2019 • edited by yao-msft

Choose a reason for hiding this comment

yao-msft Dec 20, 2019

Choose a reason for hiding this comment

JohnMcPMS left a comment

Choose a reason for hiding this comment

JohnMcPMS Dec 17, 2019 •

edited

JohnMcPMS Dec 17, 2019 •

edited

JohnMcPMS commented Dec 17, 2019 •

edited

JohnMcPMS commented Dec 17, 2019 •

edited

yao-msft commented Dec 19, 2019 •

edited by JohnMcPMS

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

ranm-msft Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft

ranm-msft Dec 19, 2019 •

edited by yao-msft

JohnMcPMS Dec 19, 2019 •

edited by yao-msft