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

Add -v and --version flags to ch to print ChakraCore version. #2490

Merged
merged 4 commits into from
Feb 9, 2017

Conversation

dilijev
Copy link
Contributor

@dilijev dilijev commented Feb 8, 2017

Partially addresses #109 and #2472.

This is intended as a first pass at the issue to provide some information at this point.

Later, we plan to add more information such as whether a binary was produced as a developer build or an official build, what specific commit the binary was built from, which branch, which machine, etc. The specific set of information that we plan to provide is still being discussed in #109.

ch /v
ch /version
ch -v
ch --v
ch -version
ch --version

(Supporting - and -- prefixes for short and long flag names is consistent with how we process all behaviors. The logic I used here is a replica of that in CmdLineArgsParser::Parse.)

The above all display the version information for ch and ChakraCore as <major>.<minor>.<patch>.<QFE> (note: by convention, ChakraCore's QFE is always 0).

For example on Windows for ChakraCore 1.4.1 with ch.exe and ChakraCore.dll from the same build:

$ ch -v
ch.exe version 1.4.1.0
chakracore.dll version 1.4.1.0

If with the same ch.exe, if ChakraCore.dll was from, e.g., a 2.0.0 build:

$ ch -v
ch.exe version 1.4.1.0
chakracore.dll version 2.0.0.0

On Linux:

~/dev/ChakraCore$ ./BuildLinux/Release/ch -v
ch version 1.4.1.0

This flag is available in all flavors (including Release).


Updating the help message. Also enable -h and -help as per request in #2472.

Enables -h and -help for non-release builds, and -?, -h, and -help for release builds.

Messages will look as follows.

debug/test builds:

Usage: ch.exe [-v|-version] [-h|-help] [-?] [flaglist] <source file>
        -v|-version             Displays version info
        -h|-help                Displays this help message
        -?                      Displays this help message with complete [flaglist] info

release builds:

Usage: ch.exe [-v|-version] [-h|-help|-?] <source file>
Note: [flaglist] is not supported in Release builds; try a Debug or Test build to enable these flags.
        -v|-version             Displays version info
        -h|-help|-?             Displays this help message

The distinction is that -? is already implemented in debug/test builds to display full information about the [flaglist] options. This is a minimal change here that will result in guiding the user more accurately in all build flavors, the effect of which is that -? in debug/test builds is still the only way to get information about the [flaglist] options.

Partially addresses chakra-core#109 and chakra-core#2472

```
ch /v
ch /version
ch -v
ch --v
ch -version
ch --version
```

The above all display the version in a simple `<major>.<minor>.<patch>` format, e.g.:

```
1.4.1
```

This flag is available in all flavors (including Release).
@dilijev
Copy link
Contributor Author

dilijev commented Feb 8, 2017

@bterlson @liminzhu We could probably merge this before the 1.4.1 release, so at a minimum 1.4.1 and 2.0.0 binaries can be distinguished at this point.
/cc @mathiasbynens @obastemur @ThomsonTan @ianwjhalliday
What do you think?

@dilijev
Copy link
Contributor Author

dilijev commented Feb 8, 2017

Problems building on xplat. What is an xplat alternative for wcsnlen -- or another way to write this?

@dilijev dilijev self-assigned this Feb 8, 2017
@dilijev dilijev added this to the 1.4.1 milestone Feb 8, 2017
@ThomsonTan
Copy link
Collaborator

Linux has wcslen. The build problem is probably about const conversion.


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

bin/ch/ch.cpp Outdated
@@ -770,7 +775,36 @@ int _cdecl wmain(int argc, __in_ecount(argc) LPWSTR argv[])
int cpos = 1;
for(int i = 1; i < argc; ++i)
{
if(wcsstr(argv[i], _u("-TTRecord=")) == argv[i])
wchar *arg = argv[i];
Copy link
Collaborator

Choose a reason for hiding this comment

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

const?

@mathiasbynens
Copy link
Contributor

We could probably merge this before the 1.4.1 release, so at a minimum 1.4.1 and 2.0.0 binaries can be distinguished at this point.

🎉

@obastemur
Copy link
Collaborator

should we include this into -? list?

@liminzhu
Copy link
Collaborator

liminzhu commented Feb 8, 2017

This sounds pretty useful. Is there a way for any embedder (not just ch) to programmatically know ChakraCore version #?

@jianchun
Copy link

jianchun commented Feb 8, 2017

This sounds pretty useful. Is there a way for any embedder (not just ch) to programmatically know ChakraCore version #?

We intend to ship "ChakraCoreVersion.h" file, and embedder knows it by including that header.

@dilijev
Copy link
Contributor Author

dilijev commented Feb 8, 2017

Yes, additionally, the file info of the compiled binaries already contains information about the version (that's how these macros were being consumed before):

Here's the info for some binaries I built locally:

ch.exe:
        Verified:       Unsigned
        Link date:      6:27 PM 2/7/2017
        Publisher:      Microsoft Corporation
        Description:    Microsoft « Chakra Core Host (Private)
        Product:        Microsoft « Chakra Core Host
        Prod version:   2.0.0.0
        File version:   2.0.0.0
        MachineType:    64-bit

ChakraCore.dll:
        Verified:       Unsigned
        Link date:      5:57 PM 2/7/2017
        Publisher:      Microsoft Corporation
        Description:    Microsoft « Chakra Core (Private)
        Product:        Microsoft « Chakra Core
        Prod version:   2.0.0.0
        File version:   2.0.0.0
        MachineType:    64-bit

And from the preview package I recently published to myget:

microsoft.chakracore.vc140.1.4.1-preview-00010-42060\lib\native\v140\x64\debug\ch.exe:
        Verified:       Signed
        Signing date:   7:17 PM 1/30/2017
        Publisher:      Microsoft Corporation
        Description:    Microsoft « Chakra Core Host
        Product:        Microsoft « Chakra Core Host
        Prod version:   1.4.1.0
        File version:   1.4.1.0 (users/doilij/nufeed:3e2e2f353d71d8e3e8522fa04b9789cf1b5c35ac.00010.42060.170130-1858)
        MachineType:    64-bit

microsoft.chakracore.vc140.1.4.1-preview-00010-42060\lib\native\v140\x64\debug\ChakraCore.dll:
        Verified:       Signed
        Signing date:   7:15 PM 1/30/2017
        Publisher:      Microsoft Corporation
        Description:    Microsoft « Chakra Core
        Product:        Microsoft « Chakra Core
        Prod version:   1.4.1.0
        File version:   1.4.1.0 (users/doilij/nufeed:3e2e2f353d71d8e3e8522fa04b9789cf1b5c35ac.00010.42060.170130-1858)
        MachineType:    64-bit

@dilijev
Copy link
Contributor Author

dilijev commented Feb 8, 2017

It looks like the problem is that the signature of main for xplat specifies the following and converts so that argv is a char16*

#ifndef _WIN32
static char16** argv = nullptr;
int main(int argc, char** c_argv)
{
    PAL_InitializeChakraCore(argc, c_argv);
    argv = new char16*[argc];

Instead of the Windows version which uses:

int _cdecl wmain(int argc, __in_ecount(argc) LPWSTR argv[])

Note LPWSTR is typedef'd to wchar_t on Windows at least.

Additionally, I confused things initially by using wchar instead of wchar_t -- apparently not a problem on Windows but it is for xplat.

wcsnlen requires wchar_t * per the standard.

@dilijev
Copy link
Contributor Author

dilijev commented Feb 8, 2017

Apparently we have PAL_wcslen but not PAL_wcsnlen -- there's an implementation here but it seems better to just copy the existing PAL_wcslen and add the additional loop bound.

On the other hand @jianchun suggests that since security is not a concern in ch, wcslen should be enough for ch -- and additionally, it may be the case that PAL is too problematic for ch and we should try to avoid using PAL directly.

@dilijev
Copy link
Contributor Author

dilijev commented Feb 8, 2017

I'm going to look into using HostConfigFlags instead to avoid introducing the PAL dependency in ch. If that works, I'll update the PR away from the current approach.

@obastemur
Copy link
Collaborator

Additionally, I confused things initially by using wchar instead of wchar_t -- apparently not a problem on Windows but it is for xplat.

We shouldn't use wchar_t I had cleaned up that one. Its size can be 4 bytes on one platform and 2 on another..

@obastemur
Copy link
Collaborator

LGTM. Thanks @dilijev for taking care of this!

@dilijev
Copy link
Contributor Author

dilijev commented Feb 9, 2017

@jianchun re: shipping ChakraCoreVersion.h -- the NuGet packages do not include this file since it's so minor. Would it be worth including it? Even so, that only solves the problem for Native projects.

For .NET projects which might want to report the ChakraCore version without hardcoding it, is there any kind of solution that would work that doesn't involve exposing a new API that reports the version info? Should we consider implementing such an API for the benefit of hosting? Is there some other approach that could work?

/cc @liminzhu

@dilijev
Copy link
Contributor Author

dilijev commented Feb 9, 2017

I'm going to look into using HostConfigFlags instead to avoid introducing the PAL dependency in ch. If that works, I'll update the PR away from the current approach.

From my investigation so far, this seems like a bit of a rabbit hole. There seems to be good reason (at least, it was thought about before) not to initialize the HostConfigFlags in release builds. Working around it to enable these specific parameters seems silly and difficult, so if we went this way, it would involve enabling all HostConfigFlags.

I agree that taking a dependency on PAL for this may be a bit extreme. One workaround is to copy a definition of PAL_wcslen to support taking length of char16 strings to avoid the dependency problem, but I'd like to avoid doing something like that if it's not totally necessary. Any suggestions?

@jianchun @obastemur Think it's safe to merge this as-is and follow-up later if the dependency becomes a problem?

@dilijev dilijev mentioned this pull request Feb 9, 2017
@dilijev
Copy link
Contributor Author

dilijev commented Feb 9, 2017

Updating the help message. Also enable -h and -help as per request in #2472.

Enables -h and -help for non-release builds, and -?, -h, and -help for release builds.

Messages will look as follows.

debug/test builds:

Usage: ch.exe [-v|-version] [-h|-help] [-?] [flaglist] <source file>
        -v|-version             Displays version info
        -h|-help                Displays this help message
        -?                      Displays this help message with complete [flaglist] info

release builds:

Usage: ch.exe [-v|-version] [-h|-help|-?] <source file>
Note: [flaglist] is not supported in Release builds; try a Debug or Test build to enable these flags.
        -v|-version             Displays version info
        -h|-help|-?             Displays this help message

The distinction is that -? is already implemented in debug/test builds to display full information about the [flaglist] options. This is a minimal change here that will result in guiding the user more accurately in all build flavors, the effect of which is that -? in debug/test builds is still the only way to get information about the [flaglist] options.

/cc @mathiasbynens @obastemur

…t in chakra-core#2472.

Enables `-h` and `-help` for non-release builds, and `-?`, `-h`, and `-help` for release builds.

Messages will look as follows.

debug/test builds:

```
Usage: ch.exe [-v|-version] [-h|-help] [-?] [flaglist] <source file>
        -v|-version             Displays version info
        -h|-help                Displays this help message
        -?                      Displays this help message with complete [flaglist] info
```

release builds:

```
Usage: ch.exe [-v|-version] [-h|-help|-?] <source file>
Note: [flaglist] is not supported in Release builds; try a Debug or Test build to enable these flags.
        -v|-version             Displays version info
        -h|-help|-?             Displays this help message
```

The distinction is that `-?` is already implemented in debug/test builds to display full information about the `[flaglist]` options. This is a minimal change here that will result in guiding the user more accurately in all build flavors, the effect of which is that `-?` in debug/test builds is still the only way to get information about the `[flaglist]` options.
@dilijev
Copy link
Contributor Author

dilijev commented Feb 9, 2017

@tcare Believes that this is potentially misleading as the version being reported is really just the version of ch, not the version of ChakraCore that ch loads. This echos an earlier concern that I had when I started working on this.

At a minimum, we should report the following at this time:

ch.exe version 1.4.1

We could read the ChakraCore.dll binary information (see above), but the ChakraCore binaries produced in xplat builds don't seem to contain this info at this time, so we would be unable to do the same for xplat right now. It may be reasonable to make this a separate work item for later.

Assuming that in the common case ch will only be used with the ChakraCore library that was built at the same time, the version of ch can be used as a proxy for the version of ChakraCore it will load, and this could mostly resolve the common scenarios that @bterlson @mathiasbynens and others are interested in.

Will discuss with others on the team for opinions about how to proceed before merging this.

@dilijev
Copy link
Contributor Author

dilijev commented Feb 9, 2017

https://stackoverflow.com/questions/940707/how-do-i-programatically-get-the-version-of-a-dll-or-exe-file/940743#940743 shows how to read file information from a binary.

Output of ch -v etc. is now:

ch.exe version 1.4.1.0
chakracore.dll version 1.4.1.0

Or, because it reads the DLL info, when the DLL built from a different version, that is reflected in the output as well:

ch.exe version 1.4.1.0
chakracore.dll version 2.0.0.0

Note: At present, this only reports the ChakraCore version on Windows. On xplat, only the ch version will be reported. Reporting the version on xplat will require additional work: embedding that information in the binary and writing an alternate implementation to retrieve that info for xplat.

/cc @tcare

@dilijev
Copy link
Contributor Author

dilijev commented Feb 9, 2017

Windows x86_release error:

warning C6388: 'verHandle' might not be '0': this does not adhere to the specification for the function 'GetFileVersionInfoA'.

DWORD
APIENTRY
GetFileVersionInfoSizeA(
        _In_        LPCSTR lptstrFilename, /* Filename of version stamped file */
        _Out_opt_ LPDWORD lpdwHandle       /* Information for use by GetFileVersionInfo */
        );
BOOL
APIENTRY
GetFileVersionInfoA(
        _In_                LPCSTR lptstrFilename, /* Filename of version stamped file */
        _Reserved_          DWORD dwHandle,          /* Information from GetFileVersionSize */
        _In_                DWORD dwLen,             /* Length of buffer for info */
        _Out_writes_bytes_(dwLen) LPVOID lpData            /* Buffer to place the data structure */
        );

What? This seems like a warning that should be suppressed because clearly the value of &verHandle output from GetFileVersionInfoSizeA is supposed to be passed into GetFileVersionInfoA, unless I'm totally misreading this.

@tcare
Copy link
Contributor

tcare commented Feb 9, 2017

dwHandle is reserved and is ignored. Just pass in nullptr. https://msdn.microsoft.com/en-us/library/windows/desktop/ms647003(v=vs.85).aspx

@dilijev
Copy link
Contributor Author

dilijev commented Feb 9, 2017

@tcare Thanks for confirming, I was thinking that might be the right thing to do but I couldn't find info about it. I didn't think to check the API page since I figured the function signature would be enough to tell why the warning happened (clearly the _Reserved_ was the bit I didn't quite understand).

bin/ch/ch.cpp Outdated
DWORD verHandle = NULL;
UINT size = 0;
LPBYTE lpBuffer = NULL;
DWORD verSize = GetFileVersionInfoSizeA(filename, &verHandle);
Copy link
Contributor

Choose a reason for hiding this comment

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

verHandle is optional for this and we don't use it later, so you can also pass nullptr here.

@@ -23,6 +23,11 @@ ChakraRTInterface::ArgInfo* ChakraRTInterface::m_argInfo = nullptr;
TestHooks ChakraRTInterface::m_testHooks = { 0 };
JsAPIHooks ChakraRTInterface::m_jsApiHooks = { 0 };

LPCSTR GetChakraDllName()
{
return chakraDllName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Top of the file. Different string for each platform.

@@ -37,6 +37,7 @@
ole32.lib;
kernel32.lib;
Rpcrt4.lib;
$(ChakraCommonLinkDependencies);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to add this link dependency? Is there a more lightweight way to get the Dll name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll just link the specific lib.

@dilijev dilijev force-pushed the version-r14 branch 2 times, most recently from 93d53e5 to 57eba6b Compare February 9, 2017 22:14
bin/ch/ch.cpp Outdated
(size != 0))
{
VS_FIXEDFILEINFO *verInfo = (VS_FIXEDFILEINFO *)lpBuffer;
if (verInfo->dwSignature == 0xfeef04bd)
Copy link
Contributor

Choose a reason for hiding this comment

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

The magic number can be VS_FFI_SIGNATURE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks, that was bugging me too.

@tcare
Copy link
Contributor

tcare commented Feb 9, 2017

LGTM, please run by @curtisman.

@chakrabot chakrabot merged commit beb522d into chakra-core:release/1.4 Feb 9, 2017
chakrabot pushed a commit that referenced this pull request Feb 9, 2017
…raCore version.

Merge pull request #2490 from dilijev:version-r14

Partially addresses #109 and #2472.

This is intended as a first pass at the issue to provide some information at this point.

Later, we plan to add more information such as whether a binary was produced as a developer build or an official build, what specific commit the binary was built from, which branch, which machine, etc. The specific set of information that we plan to provide is still being discussed in #109.

```
ch /v
ch /version
ch -v
ch --v
ch -version
ch --version
```

(Supporting `-` and `--` prefixes for short and long flag names is consistent with how we process all behaviors. The logic I used here is a replica of that in `CmdLineArgsParser::Parse`.)

The above all display the version information for `ch` and `ChakraCore` as `<major>.<minor>.<patch>.<QFE>` (note: by convention, ChakraCore's `QFE` is always `0`).

For example on Windows for ChakraCore 1.4.1 with `ch.exe` and `ChakraCore.dll` from the same build:

```
$ ch -v
ch.exe version 1.4.1.0
chakracore.dll version 1.4.1.0
```

If with the same `ch.exe`, if `ChakraCore.dll` was from, e.g., a 2.0.0 build:

```
$ ch -v
ch.exe version 1.4.1.0
chakracore.dll version 2.0.0.0
```

On Linux:

```
~/dev/ChakraCore$ ./BuildLinux/Release/ch -v
ch version 1.4.1.0
```

This flag is available in all flavors (including Release).

---

Updating the help message. Also enable `-h` and `-help` as per request in #2472.

Enables `-h` and `-help` for non-release builds, and `-?`, `-h`, and `-help` for release builds.

Messages will look as follows.

debug/test builds:

```
Usage: ch.exe [-v|-version] [-h|-help] [-?] [flaglist] <source file>
        -v|-version             Displays version info
        -h|-help                Displays this help message
        -?                      Displays this help message with complete [flaglist] info
```

release builds:

```
Usage: ch.exe [-v|-version] [-h|-help|-?] <source file>
Note: [flaglist] is not supported in Release builds; try a Debug or Test build to enable these flags.
        -v|-version             Displays version info
        -h|-help|-?             Displays this help message
```

The distinction is that `-?` is already implemented in debug/test builds to display full information about the `[flaglist]` options. This is a minimal change here that will result in guiding the user more accurately in all build flavors, the effect of which is that `-?` in debug/test builds is still the only way to get information about the `[flaglist]` options.
@dilijev dilijev deleted the version-r14 branch February 9, 2017 23:19
chakrabot pushed a commit that referenced this pull request Feb 9, 2017
…to print ChakraCore version.

Merge pull request #2490 from dilijev:version-r14

Partially addresses #109 and #2472.

This is intended as a first pass at the issue to provide some information at this point.

Later, we plan to add more information such as whether a binary was produced as a developer build or an official build, what specific commit the binary was built from, which branch, which machine, etc. The specific set of information that we plan to provide is still being discussed in #109.

```
ch /v
ch /version
ch -v
ch --v
ch -version
ch --version
```

(Supporting `-` and `--` prefixes for short and long flag names is consistent with how we process all behaviors. The logic I used here is a replica of that in `CmdLineArgsParser::Parse`.)

The above all display the version information for `ch` and `ChakraCore` as `<major>.<minor>.<patch>.<QFE>` (note: by convention, ChakraCore's `QFE` is always `0`).

For example on Windows for ChakraCore 1.4.1 with `ch.exe` and `ChakraCore.dll` from the same build:

```
$ ch -v
ch.exe version 1.4.1.0
chakracore.dll version 1.4.1.0
```

If with the same `ch.exe`, if `ChakraCore.dll` was from, e.g., a 2.0.0 build:

```
$ ch -v
ch.exe version 1.4.1.0
chakracore.dll version 2.0.0.0
```

On Linux:

```
~/dev/ChakraCore$ ./BuildLinux/Release/ch -v
ch version 1.4.1.0
```

This flag is available in all flavors (including Release).

---

Updating the help message. Also enable `-h` and `-help` as per request in #2472.

Enables `-h` and `-help` for non-release builds, and `-?`, `-h`, and `-help` for release builds.

Messages will look as follows.

debug/test builds:

```
Usage: ch.exe [-v|-version] [-h|-help] [-?] [flaglist] <source file>
        -v|-version             Displays version info
        -h|-help                Displays this help message
        -?                      Displays this help message with complete [flaglist] info
```

release builds:

```
Usage: ch.exe [-v|-version] [-h|-help|-?] <source file>
Note: [flaglist] is not supported in Release builds; try a Debug or Test build to enable these flags.
        -v|-version             Displays version info
        -h|-help|-?             Displays this help message
```

The distinction is that `-?` is already implemented in debug/test builds to display full information about the `[flaglist]` options. This is a minimal change here that will result in guiding the user more accurately in all build flavors, the effect of which is that `-?` in debug/test builds is still the only way to get information about the `[flaglist]` options.
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.

9 participants