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

ChakraCore NuGet Package Implementation #2266

Merged
merged 1 commit into from
Jan 14, 2017

Conversation

pre10der89
Copy link
Contributor

@pre10der89 pre10der89 commented Dec 22, 2016

Adding implementation for several nuget packages that allow the ChakraCore DLLs to be referenced in .NET projects... This main tasks in git issue #85...

A nuspec has been provided for the following flavors:

ARM, x86, x64 (combined)
ARM (only)
x86 (only)
x64 (only)
Symbols [PDBs] (only)

v140 (CPP only)

@msftclas
Copy link

Hi @pre10der89, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@digitalinfinity
Copy link
Contributor

@boingoing can you fix the copyright check script to handle this PR. @pre10der89, once we fix the copyright check script, you can rebase your PR on top of that change, and also update the ps1 file to change tabs to spaces.

@@ -0,0 +1 @@
2.0.2-pre
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to update this version number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dilijev I simply chose that value for internal testing... Is there a version you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

@digitalinfinity what do you think? IMO it's not necessary to change this for this PR. We will update it as needed.

@dilijev
Copy link
Contributor

dilijev commented Dec 22, 2016

Hi @pre10der89, thanks so much for this!

@@ -0,0 +1,45 @@
$root = (split-path -parent $MyInvocation.MyCommand.Definition) + '\..'
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add the Microsoft copyright header to this file?

#-------------------------------------------------------------------------------------------------------
# Copyright (C) Microsoft. All rights reserved.
# Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
#-------------------------------------------------------------------------------------------------------

@dilijev
Copy link
Contributor

dilijev commented Dec 22, 2016

@pre10der89 @boingoing @digitalinfinity all of the issues with CI can be solved in this PR, no need for a separate PR and rebase.

Here are the only files that weren't excluded form the check (we already have an exclusion for .nuspec files):

18:15:03 Check Copyright > Checking Build/NuGet/.pack-version
18:15:03 Check Copyright > Checking Build/NuGet/_._
18:15:03 Check Copyright > Checking Build/NuGet/package.ps1

.pack-version is failing because it's non-empty and doesn't have the header.

@pre10der89 We can add an exclusion for that directly in this PR by adding a line like this: https://github.com/Microsoft/ChakraCore/blob/master/jenkins/check_copyright.sh#L33

_._ is fine because the script explicitly passes empty files.

package.ps1 will need the copyright header added: #2266 (comment)

@pre10der89
Copy link
Contributor Author

@dilijev I don't think we can add the copyright to the pack-version file since the packaging script (as is) reads the entire content of the file and uses that for the package versioning...

The . file shouldn't have the copyright either because its meant to be blank...

I was able to add the copyright to the package.ps1

The use of the .pack-version and the package.ps1 is ultimately up to your build team... I utilize the .pack-version so that I only had to change the version in one location instead of in every NuSpec... The package.ps1 does build all the different flavors of packages, but doesn't go much beyond that...

@pre10der89
Copy link
Contributor Author

@dilijev I have also signed the CLA twice... I see the CLA required tag is still there, however... Perhaps just a backlog...

@dilijev
Copy link
Contributor

dilijev commented Jan 5, 2017

@pre10der89 We'll look into that, thanks! /cc @tcare @digitalinfinity

Copy link
Contributor

@dilijev dilijev left a comment

Choose a reason for hiding this comment

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

Thanks for resolving those copyright issues!

@@ -31,6 +31,7 @@ git diff --name-only `git merge-base origin/master HEAD` HEAD |
grep -v -E '\.filters$' |
grep -v -E '\.targets$' |
grep -v -E '\.nuspec$' |
grep -v -E '\.pack-version$' |
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you introduced a tab here ;P

@dilijev
Copy link
Contributor

dilijev commented Jan 5, 2017

@pre10der89 Off the top of my head, it could be that it didn't resolve your identity. It looks like your commits are using the identity Andrew Forster <anforste@bluejeansnet.com> -- can you ensure that this email address is associated with your GitHub account and the same email address you are using to sign the CLA?

If all of that is set up, we'll have to work a bit harder to figure out what piece is missing. :)

@msftgits msftgits closed this Jan 5, 2017
@msftgits msftgits reopened this Jan 5, 2017
@msftclas
Copy link

msftclas commented Jan 5, 2017

Hi @pre10der89, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@pre10der89
Copy link
Contributor Author

@dilijev Andrew Forster anforste@bluejeansnet.com is my primary email associated with my GitHub account

@dilijev
Copy link
Contributor

dilijev commented Jan 5, 2017

@pre10der89 Apparently it was a glitch with the GitHub API Hook. It's resolved now. :)

@dilijev
Copy link
Contributor

dilijev commented Jan 10, 2017

@pre10der89 Would you guys like us to release a package using these definitions for release/1.4? If so, could you retarget this pull request to release/1.4? Hit the "Edit" button at the top right and then select the new base branch as in this screenshot.

image

Then apply your changes onto release/1.4, and force-push that ref to the branch you used to open this PR.

@pre10der89 pre10der89 changed the base branch from master to release/1.4 January 10, 2017 18:30
@pre10der89
Copy link
Contributor Author

@dilijev I have changed the base branch to release/1.4 and force pushed... This created a merge conflict on one of the test files... I then rebased my local branch against master and pushed again... This seems to have cleared the conflicts and it appears the CI is running...

Let me know if I've done this right :)

commit 1325f44
Merge: 6432af3 c1623a6
Author: Andrew Forster <anforste@bluejeansnet.com>
Date:   Tue Jan 10 10:40:05 2017 -0800

    Merge branch 'master' of https://github.com/pre10der89/ChakraCore into chakra_nuget

commit 6432af3
Merge: bfec5a6 71a87b0
Author: Andrew Forster <anforste@bluejeansnet.com>
Date:   Wed Jan 4 17:44:24 2017 -0800

    Merge branch 'master' of https://github.com/pre10der89/ChakraCore into chakra_nuget

commit bfec5a6
Author: Andrew Forster <anforste@bluejeansnet.com>
Date:   Wed Jan 4 17:42:52 2017 -0800

    Replace Tab with spaces

commit b9c321c
Author: Andrew Forster <anforste@bluejeansnet.com>
Date:   Wed Jan 4 15:29:49 2017 -0800

    Added copyright and converted tabs to space in package.ps1 and added .pack-version to the ignore copyright check

commit 3a8a9a0
Merge: b486c3c 3f84a23
Author: Andrew Forster <anforste@bluejeansnet.com>
Date:   Wed Jan 4 15:16:40 2017 -0800

    Merge branch 'master' of https://github.com/pre10der89/ChakraCore into chakra_nuget

commit b486c3c
Author: Andrew Forster <anforste@bluejeansnet.com>
Date:   Wed Dec 21 17:43:50 2016 -0800

    Adding updated nuspec
@dilijev
Copy link
Contributor

dilijev commented Jan 10, 2017

@pre10der89 Looks great! Thanks for your patience and for indulging us on this adventure :)

@chakrabot chakrabot merged commit f27560f into chakra-core:release/1.4 Jan 14, 2017
chakrabot pushed a commit that referenced this pull request Jan 14, 2017
Merge pull request #2266 from pre10der89:chakra_nuget

Adding implementation for several nuget packages that allow the ChakraCore DLLs to be referenced in .NET projects... This main tasks in git issue #85...

A nuspec has been provided for the following flavors:

ARM, x86, x64 (combined)
ARM (only)
x86 (only)
x64 (only)
Symbols [PDBs] (only)

v140 (CPP only)
chakrabot pushed a commit that referenced this pull request Jan 14, 2017
…mentation

Merge pull request #2266 from pre10der89:chakra_nuget

Adding implementation for several nuget packages that allow the ChakraCore DLLs to be referenced in .NET projects... This main tasks in git issue #85...

A nuspec has been provided for the following flavors:

ARM, x86, x64 (combined)
ARM (only)
x86 (only)
x64 (only)
Symbols [PDBs] (only)

v140 (CPP only)
@pre10der89 pre10der89 deleted the chakra_nuget branch January 20, 2017 01:41
@ormico ormico mentioned this pull request Jan 31, 2017
12 tasks
chakrabot pushed a commit that referenced this pull request Feb 1, 2017
…es in prep for release of new NuGet packages.

Merge pull request #2435 from dilijev:nufeed

See #2266, #85
chakrabot pushed a commit that referenced this pull request Feb 1, 2017
…d other changes in prep for release of new NuGet packages.

Merge pull request #2435 from dilijev:nufeed

See #2266, #85
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.

6 participants