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

Update Copyright notice to remove year #3204

Merged
merged 5 commits into from Mar 5, 2017
Merged

Update Copyright notice to remove year #3204

merged 5 commits into from Mar 5, 2017

Conversation

kwiknick
Copy link
Contributor

This change is to fix Issue #3148 - Update copyright notices.
Changed the ManagedEntranceStrings resource file to take in a parameter. Then edited the ManagedEntrance.cs to grab the year using GetDate.Now and placing it in the banner variable for display when powershell is opened.

This change is to fix Issue #3148 - Update copywrite notices.
@@ -106,12 +106,13 @@ public int Start(string consoleFilePath, string[] args)
RunspaceConfigForSingleShell.Create(consoleFilePath, out warning);
}
int exitCode = 0;
string systemYear = DateTime.Now.Year.ToString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need this variable if it is used only in one place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled it into a local variable because it's being used on line 113 and 115.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I was not accurate. This variable is under the #if directive and actually used once in the result code.

@lzybkr
Copy link
Member

lzybkr commented Feb 26, 2017

Maybe we don't even need the year, that would be even simpler.

@kwiknick
Copy link
Contributor Author

kwiknick commented Feb 26, 2017 via email

@iSazonov
Copy link
Collaborator

Currently all MSFT applications use a year in copyright (cmd.exe, notepad.exe, skype...)

@lzybkr
Copy link
Member

lzybkr commented Feb 26, 2017

But they do not use a dynamic year - e.g. cmd.exe says (c) 2016 Microsoft Corporation. on my machine right now.

We can follow up, but it might just be a historical thing, or copying a pattern. There was a move to remove the copyright year from source files at one point.

@iSazonov
Copy link
Collaborator

It seems that this (year) is tied to the main release time. I agree that is internal MSFT pattern. Perhaps it has historical legal roots.

@andyleejordan
Copy link
Member

Aye, we need @joeyaiello and @SteveL-MSFT to weigh in on the legal question.

@SteveL-MSFT SteveL-MSFT self-requested a review February 28, 2017 21:11
@SteveL-MSFT
Copy link
Member

I'll follow up on this.

@kwiknick
Copy link
Contributor Author

kwiknick commented Mar 1, 2017

Thanks for all the comments and feedback. I out of habit brought my fork up to upstream/master, which, as it's supposed to, kicked off the CI builds. I didn't change any of the code I submitted. I'm just big on keeping my branch, fork in this case, up to date in order to limit merge conflicts.

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

Waiting on legal

@joeyaiello
Copy link
Contributor

Just so I understand, is the date dynamically generated at build time or runtime? If I had to guess, I think runtime is probably problematic, but that's just a hunch.

Either way, I'm sending off the mail to legal now

@SteveL-MSFT
Copy link
Member

@joeyaiello I've already started a conversation with legal

@kwiknick
Copy link
Contributor Author

kwiknick commented Mar 1, 2017

@joeyaiello It is generated at runtime, which I can see how/why It'd be problematic. If legal decides that they want the date, I'll get it to generate at build time. I should've done that in the first place.

@daxian-dbw
Copy link
Member

daxian-dbw commented Mar 2, 2017

Just found that dotnet publish with the RC4 dotnet CLI doesn't have the year in coryright banner now:

PS:53> dotnet publish -f netcoreapp1.0 -c release
Microsoft (R) Build Engine version 15.1.545.13942
Copyright (C) Microsoft Corporation. All rights reserved.

  console -> X:\arena\apps\console\bin\release\netcoreapp1.0\console.dll

powershell should go this direction too.

@joeyaiello
Copy link
Contributor

We'll ask if that's an option to legal as well

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 3, 2017

For legal too: What copyright we should use in the files? These files have been published under MIT. There are also new files.

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

We are approved to just use:

// Copyright (C) Microsoft Corporation. All rights reserved.

@kwiknick
Copy link
Contributor Author

kwiknick commented Mar 3, 2017

@SteveL-MSFT That's great news! I'll update it this evening and push the changes. Thanks everyone for all the collaboration.

@kwiknick
Copy link
Contributor Author

kwiknick commented Mar 4, 2017

@SteveL-MSFT the changes have been made and this is ready for review. Thanks for being so helpful.

#else
var banner = ManagedEntranceStrings.ShellBanner;
var banner = string.Format(ManagedEntranceStrings.ShellBanner);
Copy link
Collaborator

Choose a reason for hiding this comment

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

string.Format - It seems should be removed.

And why we use Core and non-Core banner?

@kwiknick
Copy link
Contributor Author

kwiknick commented Mar 4, 2017

@SteveL-MSFT Thanks for catching the unnecessary string.Formats. I forgot to remove them as well a couple using statements. The way I read the code when I first forked it, was that it shows a different banner based on the OS of the machine. Do you want me to remove that as well and just have it to be the same message regardless?

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 4, 2017

I remember that @lzybkr open PR to remove Windows PowerShell from the repo so I believe we should remove the non-Core banner.

@SteveL-MSFT
Copy link
Member

@iSazonov I believe that PR is to just stop building Windows PowerShell. I would leave the non-Core banner for now.

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 4, 2017

I agree.

@kwiknick kwiknick closed this Mar 4, 2017
@kwiknick kwiknick reopened this Mar 4, 2017
@msftclas
Copy link

msftclas commented Mar 4, 2017

@kwiknick,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@kwiknick
Copy link
Contributor Author

kwiknick commented Mar 4, 2017

Closed and Reopened to kick off the cla bot.

@SteveL-MSFT SteveL-MSFT changed the title Add dynamic year for managed entrance message. Update Copyright notice to remove year Mar 5, 2017
@lzybkr lzybkr merged commit df0a390 into PowerShell:master Mar 5, 2017
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.

None yet

9 participants