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 install #95

Merged
merged 4 commits into from
Aug 30, 2019
Merged

Update install #95

merged 4 commits into from
Aug 30, 2019

Conversation

jamill
Copy link
Member

@jamill jamill commented Aug 29, 2019

Several tweaks to the Scalar Distribution Installer:

  • Quote bash variables
  • Use token substitution to generate configuration for installation script
  • Configure GCM as part of scalar install so it works out of box.

More details can be found in the individual commit messages.


# Configure GCM
# GCM Core installer does not current configure itself properly in all scenarios
# Configure it here to ensure it is configured correctly
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain more about how GCM Core isn't configured appropriately? I would lean towards fixing the config problem at the source if at all possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed one issue (we need to generate and consume a new GCM Core release to pick it up), but there are still other issues that @mjcheetham was working on. Similar to why we do this workaround in the VFS for Git upgrader for Mac as well. I will see if we have an open issue for this in GCM (@mjcheetham had provided details offline at one point)

Copy link
Member

Choose a reason for hiding this comment

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

OK. That makes sense. Is there any concern about this applying to the wrong Git if this is the first time that our custom Git package has been installed and not on the PATH at the beginning of this script's execution?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any concern about this applying to the wrong Git if this is the first time that our custom Git package has been installed and not on the PATH at the beginning of this script's execution?

hmm... It looks like because we are running these with sudo, the config is being applied to the (correct) new git system. I am not sure exactly why this is /cc @jeffhost @nickgra in case they have any insight...

Copy link
Member Author

Choose a reason for hiding this comment

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

I added another commit to clear Bash's hash of command paths so it will pick up the new git

Copy link
Contributor

@jeffhostetler jeffhostetler Aug 29, 2019

Choose a reason for hiding this comment

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

you could also do something like "git version | grep 2.22.0.vfs" or "/usr/local/bin/git version | grep ..."
and confirm that it reports the known version. and maybe add a "which git" and confirm that it is
the one we want.

Copy link
Member

@jrbriggs jrbriggs left a comment

Choose a reason for hiding this comment

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

Quoting and token substitution make sense, but I don't want to do GCM Core config here unless unavoidable.

The pattern of the Scalar distribution writing out a "driver" script sets
environment variables and then calls another script can have issues when
calling the driver script from a directory other than the one that contains the
scripts. Instead, use a template for the installation script and set the
desired configuration by token replacement, which should be more robust.
@jamill jamill merged commit ef18dbf into microsoft:master Aug 30, 2019
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.

4 participants