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

WIP: Export IFW (Beta) #1734

Merged
merged 88 commits into from
Oct 4, 2017
Merged

WIP: Export IFW (Beta) #1734

merged 88 commits into from
Oct 4, 2017

Conversation

podsvirov
Copy link
Contributor

Add export to binary crossplatform repository/installer
with GUI based on QtIFW:
http://doc.qt.io/qtinstallerframework/ifw-overview.html

For correct operation of these changes,
you must use the corrected QtIFW:
https://codereview.qt-project.org/#/c/203958

Add Kinect driver to OpenNI2 port.
OpenNI2 port generate Kinect driver when Kinect SDK has been installed
in your system.
Dynamic path convert of Kinect SDK directory in patch file.
@podsvirov
Copy link
Contributor Author

podsvirov commented Aug 30, 2017

Hello everyone interested!
Today I have something to show you.
This is a stand-alone stand-alone installer created based on this change request.
Installer:
vcpkg-export-ifw-standalone.exe
Screenshot (please, do not pay attention to partial localization):
vcpkg-export-ifw.png
Screenshot for funny (how it's look on Debian 9 with KDE):
vcpkg-export-ifw-debian-kde.png
I will be glad to receive feedback and constructive suggestions.

@podsvirov
Copy link
Contributor Author

Hello @alexkaratarakis and @ras0219-msft,
what your think about add this feature?
What is the probability of accepting my changes after revision and refactoring?

Copy link
Contributor

@drdanz drdanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a great addition to me...
I did not review this code, I just noticed this typo from the screenshots.

line += "<Package>";
lines.push_back(line); line = skip;
line += "<DisplayName>";
line += "Intergation";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo here... Intergation -> Integration (same typo also a few lines later in the comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thank you!

@podsvirov
Copy link
Contributor Author

QtIFW CR203958/3 has updated.
Previousle option AllowDashInName extended and renamed to AllowAllInNameAndVersion.
Updated standalone installer:
vcpkg-export-ifw-standalone.exe
And I'm glad to announce the online installer:
vcpkg-export-ifw-online.exe
New screenshot:
vcpkg-export-ifw.png
Anyone can test these installers.
I will be glad to receive feedback and constructive suggestions.

@podsvirov
Copy link
Contributor Author

@alexkaratarakis, ping :-)
In these changes, I need to generate the files in XML format. Please tell me how to do this most effectively within the vcpkg project?

@podsvirov podsvirov force-pushed the export-ifw branch 3 times, most recently from 57b7956 to 1cc3222 Compare September 7, 2017 03:30
@podsvirov podsvirov force-pushed the export-ifw branch 2 times, most recently from 49535dd to 6f5bbdb Compare September 15, 2017 03:38
@atkawa7 atkawa7 mentioned this pull request Sep 19, 2017
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Sep 19, 2017

Sorry for the delay! This is really cool and we would love to merge it in with some more polish!

For the time being, we want to avoid external library dependencies to avoid the horrible bootstrapping problem. If there aren't too many fields, it may be clean enough to synthesize with Strings::format[1]. Alternatively, we sometimes just use regex as a glorified replace-all [2].

Given the significant size of the XML generation code, I think it would be best to break it into a separate function or even another file.

[1] https://github.com/Microsoft/vcpkg/blob/c2a735f6bee792d2419b0616eaa84abef91536e6/toolsrc/src/commands_integrate.cpp#L19-L25
[2] https://github.com/Microsoft/vcpkg/blob/c2a735f6bee792d2419b0616eaa84abef91536e6/toolsrc/src/commands_integrate.cpp#L85-L105

@podsvirov
Copy link
Contributor Author

Hello @ras0219-msft, thanks for approving my ideas. I hope that these changes will be useful for many developers.
What about the time, then we all are busy, especially if it's about open source development (as a hobby, for me now).
Recently, I continue to work on the necessary changes in the QtIFW and CR203958/9 has updated. Previousle option AllowAllInNameAndVersion now do not need at all.
@ras0219-msft, I will listen to your recommendations and try to make my code better.

Add Kinect for Windows SDK v2.x port.
@podsvirov
Copy link
Contributor Author

@ras0219-msft, today I have move my code to another files. If somewere is wrong, please correct me.

std::string export_id = create_export_id();
if (ifw)
{
export_id = "vcpkg-export"; // TODO: Remove after debugging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Remove after debugging

if (ifw)
{
// Export real package and return data dir for installation
spec_exported_dir_path = IFW::export_real_package(raw_exported_dir_path, action, fs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this handle multiple export formats at the same time (i.e. --ifw --nuget --raw)? I think either the from_destination_root() call needs to be duplicated or you could add a check that requires --ifw to not be passed with any other export types.

fs.create_directories(destination.parent_path(), ec);
Checks::check_exit(VCPKG_LINE_INFO, !ec);
fs.copy_file(source, destination, fs::copy_options::overwrite_existing, ec);
Checks::check_exit(VCPKG_LINE_INFO, !ec);
}

if (ifw)
{
// Unigue packages
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

// Integration
IFW::export_integration(raw_exported_dir_path, fs);
// Configuration
IFW::export_config(raw_exported_dir_path, maybe_ifw_repository_url.value_or(""), fs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the IFW path really isn't able to reuse much or anything from the main loop -- it might make sense to completely extract it from the loop and just have

IFW::export(export_plan, raw_exported_dir_path, fs);

Then, we'd completely skip the main loop when only running --ifw.

Note that I do think the functions should probably stay separated, but they'd be private to the IFW implementation resulting in a "cleaner" public interface.

@@ -417,7 +460,7 @@ With a project open, go to Tools->NuGet Package Manager->Package Manager Console
print_next_step_info("[...]");
}

if (!raw)
if (!raw && !ifw)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this was for debugging?

@ras0219-msft
Copy link
Contributor

I pushed a commit that switches to using sprintf-style templates instead of line-by-line, which makes the code significantly more compact and (at least for me) easier to read.

@podsvirov
Copy link
Contributor Author

Hello @ras0219-msft, thanks for your comments and commit. Now I will correct my mistakes.

@podsvirov
Copy link
Contributor Author

CppCon is done... Ping :-)

atkawa7 and others added 12 commits October 4, 2017 05:36
Extract WiX installer using Dark.
It will be standalone extract files from installer of Kinect SDK 1.x
even if Kinect SDK 1.x is not installed in user system.
Fix according to changes of Kinect SDK 1.x port.
Change to refer Kinect SDK 1.x that installed using vcpkg port.
It will be always generate the Kinect SDK 1.x driver, even when Kinect
SDK 1.x is not installed in user system.
Extract Kinect SDK 1.x Installer using Dark
Change to refer Kinect SDK 1.x that installed using vcpkg port
@ras0219-msft ras0219-msft merged commit 875a126 into microsoft:master Oct 4, 2017
@ras0219-msft
Copy link
Contributor

Thanks again for all this work, it's really incredible!

I've merged for now (with a few refactorings/housekeepings of my own); I know you have some follow-up improvements and I'd be happy to handle them through separate PRs!

@podsvirov
Copy link
Contributor Author

@ras0219-msft, thank you for merging my changes. It was really unexpected for me now, but I'm very glad that it happened. Of course I have plans for further improvement and I will do this work consistently on.

@podsvirov podsvirov deleted the export-ifw branch October 4, 2017 23:50
@podsvirov
Copy link
Contributor Author

podsvirov commented Oct 5, 2017

@ras0219-msft, the problem of the first export is still relevant to me:

A suitable version of installerbase was not found (required v3.1.81). Downloadin
g portable installerbase v3.1.81...
Expected dependency downloaded path to be J:\build\vcpkg\downloads\QtInstallerFr
amework-win-x86\bin\installerbase.exe, but was ?У?З?ТA¬?ТNЧJ:\build\vcpkg\downlo
ads\QtInstallerFramework-win-x86\bin\installerbase.exe

Unexpected symbols ?У?З?ТA¬?ТNЧ...
But the tools are downloaded and unpacked normally and on subsequent attempts work as expected.

@ras0219-msft
Copy link
Contributor

Hmm, we had some issues with byte-order marks before and were able to solve them with [1].

Could you put a breakpoint there, repro the issue, and let us know what the exact byte values are at the beginning of the response?

[1] https://github.com/Microsoft/vcpkg/blob/d8507cd159f8de6ae174cae9894c926f75fe50d7/toolsrc/src/vcpkg_System.cpp#L180-L188

@podsvirov
Copy link
Contributor Author

podsvirov commented Oct 5, 2017

I reproduced this and here is my result (I see 6 extra characters):
vcpkg-export-ifw-unexpected-symbols

@ras0219-msft
Copy link
Contributor

Thanks, that's really helpful! It appears you're getting two byte order marks (because your bytes weren't sufficiently ordered, I guess? 😋).

I've pushed 9b0c2cb to master which changes that function to loop until we no longer find any BOMs; I think this will solve your issue.

@podsvirov
Copy link
Contributor Author

@ras0219-msft thanks, it seems that now everything works as it should.

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.