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

Merging installer changes required for autoupdate to function #1

Merged
merged 14 commits into from
Apr 6, 2018

Conversation

gavankar
Copy link

These commits contain all the installer changes required on win and mac for autoupdate

@@ -46,6 +46,16 @@
<key>type</key>
<string>file</string>
</dict>
<dict>
Copy link
Owner

Choose a reason for hiding this comment

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

@gavankar can you verify this layouting change and see if it's correct. The DMG GUI layout is getting messed up on my machine when I simply mount it. The container bounds seem to be off.

image

@ayushrathi15 can you confirm this issue?

Copy link
Author

Choose a reason for hiding this comment

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

This layout change was added after this bug was found, to fix this bug

@@ -15,6 +15,12 @@ rm -rf $tempDir
mkdir $tempDir
cp -r "./staging/${BRACKETS_APP_NAME}.app/" "$tempDir/$appName"

# copy update.sh to hidden location in the dmg
if [ -f ./update.sh ]; then
echo "Adding update.sh"
Copy link
Owner

Choose a reason for hiding this comment

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

@gavankar, it would be safer to give the file "executable" permissions(chmod) programmatically before copying. Since we would simply be invoking the script later on, we don't want to end up in a situation where the script is present but is not executable due to any number of reasons.

Copy link
Author

Choose a reason for hiding this comment

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

Added in commit de725a8

}
else
{
session["UPDATEPATH"] = "0";
Copy link
Owner

Choose a reason for hiding this comment

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

@gavankar In case we are unable to read the values from registry, I am seeing that we're defaulting to 0. Has this workflow been confirmed with upper management. Because the default values in case of a fresh launch I think is 1.

@ayushrathi15 can you confirm this as well?

Copy link
Author

@gavankar gavankar Apr 5, 2018

Choose a reason for hiding this comment

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

@mbhavi Adding Brackets to the system path is an optional feature in the installer.
This piece of code is used to figure out what an upgrading user chose the last time they ran the installer, not what the option should be for a new user.
If we see that Brackets is already on the path, updatepath is set to 1 and we continue to keep Brackets on the path. If brackets is not on the path then, we shouldn't add it now and updatepath is set to 0.

Copy link
Owner

Choose a reason for hiding this comment

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

@gavankar , I think my comment was misunderstood. What i meant was, if we are unable to read the registry, irrespective of whatever value it contains, what are we doing with the registry entries? Do we default them back to 0, or do we don't do anything? If we don't touch the registry entries, does the installer at the time of update, pick up the already existing entries, so that the user selected options are kept intact for the new version as well?
How are you handling this case here?

Copy link
Author

Choose a reason for hiding this comment

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

@mbhavi, in this piece of code we are not directly reading entries from the registry. We are asking the msi installer for a copy of the entries that it has loaded as part of its execution, this is very unlikely to fail.

@mbhavi mbhavi merged commit 1be99a5 into mbhavi:miglani/autoUpdate Apr 6, 2018
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.

3 participants