Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Auto Update Framework #634

Merged
merged 27 commits into from
Apr 18, 2018
Merged

Auto Update Framework #634

merged 27 commits into from
Apr 18, 2018

Conversation

mbhavi
Copy link
Contributor

@mbhavi mbhavi commented Mar 27, 2018

This PR includes changes for Auto Update of Brackets.
The changes are with respect to:

  1. Windows implementation
  2. Mac implementation

ping @swmitra @nethip for review

@@ -507,7 +508,35 @@ class ProcessMessageDelegate : public ClientHandler::ProcessMessageDelegate {
error = CopyFile(src, dest);
// No additional response args for this function
}
} else if (message_name == "GetDroppedFiles") {
} else if (message_name == "SetUpdateParamsAndRunUpdate") {
// Parameters:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please fix the indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in ad0d490

@@ -507,7 +508,35 @@ class ProcessMessageDelegate : public ClientHandler::ProcessMessageDelegate {
error = CopyFile(src, dest);
// No additional response args for this function
}
} else if (message_name == "GetDroppedFiles") {
} else if (message_name == "SetUpdateParamsAndRunUpdate") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are you calling this method from in your extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is called from initiateUpdateProcess() in main.js file, from the extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbhavi Can you help me point to the exact location in your PR adobe/brackets#14177. You could click on the line where this is getting called and copy paste the URL from the browser, here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @mbhavi Got it!

NSArray* pArgs = [NSArray arrayWithObjects:@"detach", mountPoint, @"-force", nil];

int retval = RunScript(hdiPath, pArgs, true);
while (retval != 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When do you exit from the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The loop is exited when any prior dmgs mounted on the mount point /Volumes/BracketsAutoUpdate are unmounted. Before I mount the new dmg, I unmount any dmgs mounted already on the same mountpoint.

@mbhavi
Copy link
Contributor Author

mbhavi commented Apr 9, 2018

@nethip , I have addressed and committed all the required changes. Also, I have replied to all the comments.
The PR is ready for review again.

@lokwani
Copy link

lokwani commented Apr 9, 2018

Installer changes LGTM

"flatten" : true,
"expand" : true,
"src" : [
"installer/win/LaunchBrackets/LaunchBrackets/bin/Release/LaunchBrackets.CA.dll",
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 the source for these DLLs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore this comment.

Sequence="execute"
Value="[%PATH]" />

<Binary Id="InstallLocationChecker.dll" SourceFile="BracketsConfigurator.CA.dll" />
Copy link
Contributor

Choose a reason for hiding this comment

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

The source for this and how these DLLs are built?

Copy link
Contributor

Choose a reason for hiding this comment

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

Found them below. Please ignore the comment.

@@ -0,0 +1,67 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gavankar , please address this

Copy link
Contributor

@gavankar gavankar Apr 9, 2018

Choose a reason for hiding this comment

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

@nethip we have never had a copyright in the installer files so far(installer/win/Brackets.wxs), should we be adding one now? This file creates a customhook dll in the installer

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please go ahead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gavankar , please address this

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbhavi, Please accept the following pull request which has this fix -
mbhavi#2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gavankar , I have merged your PR. @nethip , please find the review comments addressed here, as part of this merge.

@@ -0,0 +1,30 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gavankar , please address this.

Copy link
Contributor

@gavankar gavankar Apr 9, 2018

Choose a reason for hiding this comment

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

@nethip we have never had a copyright in the installer files so far(installer/win/Brackets.wxs), should we be adding one now? This file creates a customhook dll in the installer

@mbhavi
Copy link
Contributor Author

mbhavi commented Apr 10, 2018

@nethip , addressed review comments and making auto update no-op on Linux, in 3f6d92d. The review can be resumed now.

Adding a license to the top of the Windows customaction files
@nethip
Copy link
Contributor

nethip commented Apr 10, 2018

@mbhavi @gavankar Are the custom action DLLs like BracketsExecutor part of this PR? Can you create a wiki post merging this PR as to what the steps to generate the DLLs.

Also @mbhavi post merging this PR, can you raise a PR with unit tests for the auto update process?

@gavankar
Copy link
Contributor

@nethip, the steps to create BracketsConfigurator.CA.dll and LaunchBrackets.CA.dll have been added to the "build-win" grunt task, these steps are completely automated through grunt. Do we still require a wiki about the steps for generating the dlls?

@nethip
Copy link
Contributor

nethip commented Apr 10, 2018

@gavankar Thanks for pointing out! Pretty neat! No need of adding a wiki for this.

This PR looks good. LGTM

@mbhavi Thanks for your efforts in making this happen and being patient in addressing the review comments 😄 . Really appreciate it. And great job 💯 Please address the review comments in Brackets as well. Thanks!

I would merge this after @ayushrathi15 confirms the results from sanity testing.

@mbhavi mbhavi changed the title [WIP] : Auto Update Framework Auto Update Framework Apr 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants