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

Add the RLS to .exe, .msi, and .pkg installers #42306

Merged
merged 3 commits into from
Jun 1, 2017
Merged

Add the RLS to .exe, .msi, and .pkg installers #42306

merged 3 commits into from
Jun 1, 2017

Conversation

efyang
Copy link
Contributor

@efyang efyang commented May 30, 2017

This directly addresses issue #42157, adding the RLS as a non-default component in the mentioned installers. The windows installers appear to have the right functionality added, but I don't have a machine that runs OSX, so it would be great if someone could test whether my .pkg commit adds the RLS correctly. The final commit also fixes some formatting issues I'd noticed while working on the installers, but I don't know if that's within the scope of this PR, so input would be appreciated.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nikomatsakis
Copy link
Contributor

I don't know enough about this to review.

r? @brson -- maybe you do?

@rust-highfive rust-highfive assigned brson and unassigned nikomatsakis May 30, 2017
@nikomatsakis
Copy link
Contributor

r? @alexcrichton -- actually, this is what GitHub suggests.

@alexcrichton
Copy link
Member

Thanks for the PR @efyang, it's looking great!

I tested out the pkg installer locally and it looks like it's not working though unfortunately :(. Oddly enough it looks like it installed nothing, so I'm not really sure what's going on with it... 

Have you verified the windows installers work locally?

@efyang
Copy link
Contributor Author

efyang commented May 30, 2017

Yeah, the windows installers work fine locally, and have the RLS set to non-default correctly. I've tried them with and without checking the RLS and they all appear to work fine.

@alexcrichton
Copy link
Member

Ok nice! Mind double checking the pkg installer scripts to make sure everything checks out? If they still look good I'll try the existing pkg installers and see if they work at all.

@efyang
Copy link
Contributor Author

efyang commented May 31, 2017

I just double checked them, and I believe the pkg installer scripts should work, assuming that the original pkg installer scripts work too. I might be missing something here that I might be able to find in the apple docs, but I think checking the existing pkg installers would be helpful.

@alexcrichton
Copy link
Member

Ok I just checked last night's pkg and it worked ok (installed what I expected) but double checked the pkg I produced yesterday from this branch and it still didn't install anything :(

@alexcrichton
Copy link
Member

Hm in the logs I'm seeing:

May 31 12:53:18 acrichton-23691 installd[531]: ./postinstall: /tmp/PKInstallSandbox.R8iFgf/Scripts/org.rust-lang.rustc.6Hv881/install.sh: line 11: 
	: command not found
May 31 12:53:18 acrichton-23691 installd[531]: ./postinstall: /tmp/PKInstallSandbox.R8iFgf/Scripts/org.rust-lang.rustc.6Hv881/install.sh: line 13: set: -
	: invalid option
May 31 12:53:18 acrichton-23691 installd[531]: ./postinstall: set: usage: set [--abefhkmnptuvxBCHP] [-o option] [arg ...]

@alexcrichton
Copy link
Member

Apparently for whatever reason that files has CRLF line endings locally but just LF endings online, and I guess the builders check out the submodule with LF line endings? In any case that sounds like it's some weird local problem on my end.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 31, 2017

📌 Commit f3b29d3 has been approved by alexcrichton

@aidanhs aidanhs assigned alexcrichton and unassigned brson Jun 1, 2017
@aidanhs aidanhs added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 1, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 1, 2017
Add the RLS to .exe, .msi, and .pkg installers

This directly addresses issue rust-lang#42157, adding the RLS as a non-default component in the mentioned installers. The windows installers appear to have the right functionality added, but I don't have a machine that runs OSX, so it would be great if someone could test whether my .pkg commit adds the RLS correctly. The final commit also fixes some formatting issues I'd noticed while working on the installers, but I don't know if that's within the scope of this PR, so input would be appreciated.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 1, 2017
Add the RLS to .exe, .msi, and .pkg installers

This directly addresses issue rust-lang#42157, adding the RLS as a non-default component in the mentioned installers. The windows installers appear to have the right functionality added, but I don't have a machine that runs OSX, so it would be great if someone could test whether my .pkg commit adds the RLS correctly. The final commit also fixes some formatting issues I'd noticed while working on the installers, but I don't know if that's within the scope of this PR, so input would be appreciated.
bors added a commit that referenced this pull request Jun 1, 2017
Rollup of 9 pull requests

- Successful merges: #42136, #42275, #42286, #42297, #42302, #42306, #42314, #42324, #42347
- Failed merges:
@bors bors merged commit f3b29d3 into rust-lang:master Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants