Moving the build scripts out to the public repo and other cleanups #1282

shana · 2017-10-26T18:51:33Z

It's much easier for everyone if the build scripts are out in the public so everyone can use them, and all CI systems use the same scripts so that things aren't missed.

This:

Adds a public key to the repo, with a different name from what we suggested before so it doesn't conflict with any existing keys user may have locally
Adds all build scripts to the scripts directory
Sets the version of the vsix to a timestamp when building locally
Sets the version of DLLs and vsix when building in CI, but doesn't commit them
Cleans up csproj files so all have the same CA and warnings settings
Adds solution configurations for different variations of building with CA enabled and disabling vsix creation/deployment
- Debug now builds with warnings as errors enabled but CA disabled
Builds in CI from our own PR branches and from master produced ready-to-release signed vsix packages as artifacts
Deployments are enabled in AppVeyor to S3 and GitHub releases (go to a branch with artifacts and click Deploy to deploy them). S3 deploys become available in https://ghfvs-installer.github.com/releases/X.X.X.X/[name of vsix/md5/manifest file] and https://ghfvs-installer.github.com/releases/ghfvs-X.X.X.X.zip

…iguration

grokys

A few comments, but also I'm getting:

error CS0006: Metadata file 'D:\projects\VisualStudio\submodules\reactiveui\ReactiveUI.Testing\bin\Debug\Net45\ReactiveUI.Testing.dll' could not be found

This assembly seems to be referenced from UnitTests but we're not actually building it now. It looks like the reference can safely be removed however.

grokys · 2017-11-02T10:05:57Z

src/GitHub.InlineReviews/SampleData/DiffCommentThreadViewModelDesigner.cs

    {
        public string DiffHunk { get; set; }
        public int LineNumber { get; set; }
+        public event EventHandler Disposed = delegate { };


This member can just be removed: it's not implementing an interface or anything.

grokys · 2017-11-02T10:08:35Z

src/GitHub.InlineReviews/Models/PullRequestSessionFile.cs

+
+                if (disposing)
+                {
+                    linesChanged.Dispose();


A PullRequestSessionFile can be a shared instance between multiple clients, disposing of linesChanged here would unsubscribe all clients. IMO this class shouldn't be disposable, or at least the Dispose method shouldn't dispose linesChanged as this will break things.

grokys · 2017-11-02T10:10:33Z

src/GitHub.InlineReviews/Peek/InlineCommentPeekResult.cs


        public void Dispose()
        {
+            Disposed?.Invoke(this, EventArgs.Empty);


I don't think = delegate { }; is needed as Disposed is actually called.

All use of delegate { } gone now.

grokys · 2017-11-02T10:20:17Z

src/GitHub.InlineReviews/Peek/InlineCommentPeekResultPresentation.cs

-        public event EventHandler IsDirtyChanged;
-        public event EventHandler IsReadOnlyChanged;
-        public event EventHandler<RecreateContentEventArgs> RecreateContent;
+        public event EventHandler IsDirtyChanged = delegate { }; // IsDirty is always false.


I think I'd prefer these unused events to be declared as explicit interface implementations with empty add and remove blocks, even though it's a bit more verbose - this syntax doesn't make clear to me what is happening...

Indeed. It's concise, but not exactly clear!

grokys · 2017-11-02T10:28:46Z

src/GitHub.InlineReviews/Services/PullRequestSession.cs

    /// repository on disk and in the editor.
    /// </remarks>
-    public class PullRequestSession : ReactiveObject, IPullRequestSession
+    public class PullRequestSession : ReactiveObject, IPullRequestSession, IDisposable


Again, this is a shared object so shouldn't be getting disposed.

grokys · 2017-11-02T10:32:52Z

src/GitHub.TeamFoundation.14/Connect/GitHubConnectSection.cs


-            var msg = string.Format(CultureInfo.CurrentUICulture, Constants.Notification_RepoCreated, newrepo.Name, newrepo.CloneUrl);
-            msg += " " + string.Format(CultureInfo.CurrentUICulture, Constants.Notification_CreateNewProject, newrepo.LocalPath);
+            var msg = String.Format(CultureInfo.CurrentCulture, Constants.Notification_RepoCreated, newrepo.Name, newrepo.CloneUrl);


Nit: the capitalization of string probably shouldn't have been changed here.

Leave `StartsTrueBecomesFalseWhenCompleted` as an intermediately failing test for now.

…ub/VisualStudio into shana/build-system-overhaul

jcansdale · 2017-11-02T16:31:10Z

I've fixed all but one of the MSB3277: Found conflicts between different versions of the same dependent assembly warnings.

warning MSB3277: Found conflicts between different versions of the same dependent assembly that could not be resolved.
These reference conflicts are listed in the build log when log verbosity is set to detailed.
[C:\source\github.com\github\VisualStudio\src\GitHub.VisualStudio\GitHub.VisualStudio.csproj]

Unfortunately I'm not sure there's anything we can do about this one, due to the way VSIX packaging works. I think the GitHub.VisualStudio project does need to reference all the other projects. 😢

grokys

Changes look good @jcansdale . Just one q for @shana.

grokys · 2017-11-02T17:04:45Z

appveyor.yml

-    scripts\Run-Nunit.ps1 TrackingCollectionTests 180 Release -AppVeyor
-
-    scripts\Run-Xunit.ps1 UnitTests 180 Release -AppVeyor
+- ps: scripts\test.ps1 -AppVeyor


@shana do we want to use appveyor's automatic test detection? https://www.appveyor.com/docs/running-tests/#selecting-assemblies-andor-categories-to-test

Yeah, we just need to consolidate all the tests in one place so we can have AppVeyor run them with the correct categories ignored. We also need to consolidate the test categories to be the same for everyone.

…-overhaul Backport of #1282 Build system overhaul to the 2.3.4 tree

shana added 28 commits October 28, 2017 19:00

Moving all the useful build scripts bits to public

f5c4593

Removing old files, adding dependencies, fixing appveyor

5b6d17f

Consolidate NSubstitute versions, add nunit console runner

615ca0a

Bump submodule to pick up build changes

9931550

Add public signing key for public builds

4959bf4

Add nunit2 driver package so nunit3 can run these tests

9b4e38d

Move this out to the public too

49b2418

Update solution links to script files

039b3f1

Remove experimental flag

c756182

Bump submodule

821ca9b

Make this a switch as it should be

a05a026

Bleh typo

a0f2be1

Need these debugging tools too

9e316e8

Kinda should set exitCode :P

8047648

Bump submodule for build fix

b127aa7

More cleanups

70dee92

Split build and testing and clean up the scripts to the essentials

7ecff12

Need to exit when the build fails

93c6711

Fix appveyor

aad1b54

Doh

8691ac7

Don't need this in the console

9b5e417

We can use appveyor's nunit for this

96377ce

Using the same nunit console for local and appveyor and everyone...

ee7e6ef

Set exit codes when tests fail

1f91261

Add configurations to not create a vsix when just building

9f790a3

Disable code analysis on debug builds and add debug+codeanalysis conf…

1974605

…iguration

Versioning

46b733d

Fix appveyor...

d0ebe13

shana force-pushed the shana/build-system-overhaul branch from f0c11f9 to d0ebe13 Compare October 28, 2017 17:21

Add missing file

88ed537

shana added 3 commits November 1, 2017 13:41

Fix the merge, clean up and standardize csproj files

3a4f6e6

Set version in appveyor.yml too so it can use it

29911ba

Remove dead code

24a3b9f

shana added the Ready for Review label Nov 2, 2017

shana and others added 2 commits November 1, 2017 20:17

Fix public key name

3e0f756

Fix 'WiX' is not recognized as the name of a cmdlet warning

c2c40c4

grokys suggested changes Nov 2, 2017

View reviewed changes

jcansdale and others added 3 commits November 2, 2017 11:15

Consistently reference ComponentModelHost 4.0 (Fix MSB3277)

f3b02b2

Remove StartsTrueBecomesFalseWhenCompleted_MultipleTimes

2f65d74

Leave `StartsTrueBecomesFalseWhenCompleted` as an intermediately failing test for now.

Merge branch 'shana/build-system-overhaul' of https://github.com/gith…

a1ce93d

…ub/VisualStudio into shana/build-system-overhaul

jcansdale force-pushed the shana/build-system-overhaul branch from 0a56e6d to a1ce93d Compare November 2, 2017 13:08

grokys and others added 7 commits November 2, 2017 14:18

Removed unused reference.

5d9b189

GitHub.TeamFoundation.15 should reference Shell.15.0 (fix MSB3277)

27af6ba

Change to System.Net.Http 4.1.0 (fix MSB3277)

15ed973

linesChanged is sharred and shouldn't be disposed

1c4bdc9

Remove all use of event = delegate { }

3d00bf7

PullRequestSession is shared and shouldn't be disposed

4d4a864

Make string capitalization consistent

9365c54

jcansdale closed this Nov 2, 2017

shana reopened this Nov 2, 2017

grokys approved these changes Nov 2, 2017

View reviewed changes

shana merged commit e0e5ded into master Nov 3, 2017

shana deleted the shana/build-system-overhaul branch November 3, 2017 05:54

jcansdale mentioned this pull request Nov 8, 2017

WIP: Consolidate unit tests and test runners #1278

Closed

meaghanlewis mentioned this pull request Nov 10, 2017

Add GitHub option back to start page in VS2017 #1310

Closed

shana added a commit that referenced this pull request Nov 15, 2017

Backport of #1282 Build system overhaul to the 2.3.4 tree

2975da8

shana mentioned this pull request Nov 15, 2017

Backport of #1282 Build system overhaul to the 2.3.4 tree #1320

Merged

shana added a commit that referenced this pull request Nov 15, 2017

Backport of #1282 Build system overhaul to the 2.3.4 tree

0372bbe

shana added a commit that referenced this pull request Nov 15, 2017

Merge pull request #1320 from github/backport/2.3.4/1282-build-system…

b7c5b41

…-overhaul Backport of #1282 Build system overhaul to the 2.3.4 tree

Moving the build scripts out to the public repo and other cleanups #1282

Moving the build scripts out to the public repo and other cleanups #1282

Uh oh!

Conversation

shana commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grokys left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcansdale commented Nov 2, 2017

Uh oh!

grokys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shana commented Oct 26, 2017 •

edited

Loading

grokys left a comment •

edited

Loading