Skip to content

Conversation

@Dzejkop
Copy link
Contributor

@Dzejkop Dzejkop commented Jul 25, 2018

No description provided.

@Dzejkop Dzejkop self-assigned this Jul 25, 2018
@Dzejkop Dzejkop requested review from genail and witcher112 July 25, 2018 10:53
@Dzejkop Dzejkop changed the title Implementation 1010 setting the ld_library_path on linux systems Jul 25, 2018
Copy link
Contributor

@witcher112 witcher112 left a comment

Choose a reason for hiding this comment

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

Some questions.

string launchScriptPath = LaunchScriptPath(buildPath);
string launchScriptContent = File.ReadAllText(LaunchScriptContentFile);

File.WriteAllText(launchScriptPath, launchScriptContent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just copy the file?

Target = "sh",
Arguments = new Manifest.Argument[] {
new Manifest.Argument { Value = new string[] {
"{exedir}" + launchScript
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that {exedir} ends with slash?

patcherExe,
"{secret}",
"{installdir}"
}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why there are four arguments bundled in one structure?

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 first argument:
"{exedir}" + launchScript is the shell script itself, I figured it'd be fitting to put it in a separate structure.

These four arguments are the required script arguments.

And below that there's also the optional lockfile argument.

It's basically a stylistic choice

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, if it works then it's all right.

@witcher112 witcher112 merged commit d60f37a into dev/v3.10.0 Jul 25, 2018
@witcher112 witcher112 deleted the feature/v3.10.0/1010-setting-ld-library-path branch July 25, 2018 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants