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.

Sign up for GitHub

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

Jump to bottom

Detect platform identifier with init script #60

Merged
merged 13 commits into from
Dec 30, 2019
Merged

Detect platform identifier with init script #60

merged 13 commits into from
Dec 30, 2019

Conversation

Theodus
Copy link
Contributor

@Theodus Theodus commented Dec 28, 2019 •
edited by SeanTAllen
Loading

Detect platform using a .platform config file created by the init script. This also cleans up much of the code for the show command and adds tests for the platform string parsing.

With this change, supplying --platform=musl on musl-based Linux distros like Alpine should no longer be required as "musl-based" should be detected when ponyup is installed.

If ponyup is upgraded from an earlier version without using the init script, then this change will have no impact and --platform=musl will continue to be needed.

@Theodus Theodus requested a review from SeanTAllen December 28, 2019 19:39
@Theodus Theodus added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Dec 28, 2019
@SeanTAllen
Copy link
Member

There are a couple of issues right now:

  • Detect isn't working on macOS.
  • bootstrap test isn't installing in the usual /root/.local/ on Linux. the install location isn't in the PATH so stable can't be found.

SeanTAllen
SeanTAllen reviewed Dec 28, 2019
View reviewed changes
Copy link
Member

@SeanTAllen SeanTAllen left a comment

Choose a reason for hiding this comment

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

Rather than true/false for glibc, can this be:

check for glibc: true/false
check for musl: true/false

If neither comes up, we error out?

@SeanTAllen
Copy link
Member

@Theodus are you thinking with this that we have 2 ponyup packages for Linux, one for musl, one for gnu and folks download the one that is appropriate for their distro?

@SeanTAllen SeanTAllen changed the title Detect libc at compile time Linux: detect libc at compile time Dec 28, 2019
@SeanTAllen SeanTAllen force-pushed the detect-libc branch 13 times, most recently from 532bed7 to 7ff7df5 Compare December 28, 2019 21:39
@SeanTAllen
Break out Linux bootstrap into its own environment
a820564 
We want to do bootstrap in a completely Pony free environment. Previously,
it was being done in an environment that had Pony installed. It probably
wouldn't ever be a problem, but this is a better test setup.
@SeanTAllen SeanTAllen force-pushed the detect-libc branch 2 times, most recently from 5b2833d to 38a624c Compare December 28, 2019 22:08
@SeanTAllen
Copy link
Member

SeanTAllen commented Dec 29, 2019 •
edited
Loading

@Theodus I'm having reservations about this.

The problem:

We need to link to libSSL. If we also link to Glibc, then we have to dynamically link the SSL lib. The statically linked binaries are very nice. Having a single ponyup is very nice. I think that linking against Glibc might be worse than having musl users have to give their platform. Rustup defaults to Glibc and requires you to explicitly request musl.

I like the bootstrap testing changes that have been added to this PR. I think we should definitely keep them in some form (they already found a bug).

Ideas:

  • Leave as-is with having to supply platform to get musl version of Pony
  • On Linux, in the ponyup directory, store some configuration for the platform to use. If config isn't set, then we prompt the user to supply it unless --platform was explicitly provided. If provided, we could save the selected platform and use it as the default in the future. Given that ponyup-init.sh is a shell script, we could try to get cc -dumpmachine info at the time ponyup is installed and write out the configuration if possible then.
  • Shell out to cc -dumpmachine until such time as we don't use cc for linking.

@Theodus
Copy link
Contributor Author

Theodus commented Dec 29, 2019

@SeanTAllen I agree with your concerns. I'll remove the libc detection bits from this PR and add go forward with expecting the user to provide the libc in --platform.

@Theodus
Copy link
Contributor Author

Theodus commented Dec 29, 2019

Ok, you do it

@SeanTAllen
Copy link
Member

@Theodus ok, the temporary -musl bootstrap test is in, once this is merged, it can be removed via PR after the next nightly to verify everything fully works.

@SeanTAllen SeanTAllen added changelog - added Automatically add "Added" CHANGELOG entry on merge and removed changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge labels Dec 29, 2019
@SeanTAllen
Copy link
Member

@Theodus I updated the PR comment with additional info. If there is anything else you think should be added to it, please do and we can use that comment as the commit comment when we squash and merge.

@Theodus
Copy link
Contributor Author

Theodus commented Dec 29, 2019

Looks good to me. I should probably remove the bit about needing --platform on musl from the README

@SeanTAllen
Copy link
Member

@Theodus ya and the README update for ponyc, I'll update that as well. And we can merge that after the next ponyup release (which I plan to do before the end of the year).

@SeanTAllen
Copy link
Member

@Theodus should the bootstrap test be running make test or just make. I'm not sure what we get out of doing make test. I'm thinking we should only do make.

@Theodus
Copy link
Contributor Author

Theodus commented Dec 29, 2019

Just make seems fine to me

@SeanTAllen
Copy link
Member

@Theodus are these failures at this point because not all the tools have macOS releases yet? The error messages are really obscure at this point, we should definitely improve them. It's unclear to me what the failures are.

@Theodus
Copy link
Contributor Author

Theodus commented Dec 29, 2019 •
edited
Loading

They are a result of the packages not being available, yes

@SeanTAllen
Copy link
Member

@Theodus what do you think of breaking the tests apart so that release and nightly are different? that would be a bit more clear.

it appears that release tests are hardcoded to specific versions. what is the idea with that? is it recent releases, any releases, all releases? what is your thought for those.

@Theodus
Copy link
Contributor Author

Theodus commented Dec 30, 2019

Only the select test has hard coded versions. The sync tests are a port from the shell script and uses the 2 most recent nightly and release builds.

@SeanTAllen
Copy link
Member

What about ifdefing the tests that will fail on macOS for now and create an issue to update them once they will pass by removing the ifdef and updating the versions?

@Theodus
Copy link
Contributor Author

Theodus commented Dec 30, 2019

Good idea, I always forget that pony has ifdefs

@Theodus
Copy link
Contributor Author

Theodus commented Dec 30, 2019

All CI tests are passing now except for the macOS bootstrap (which we expect to fail)

@SeanTAllen SeanTAllen merged commit c7ef0f4 into master Dec 30, 2019
@SeanTAllen SeanTAllen deleted the detect-libc branch December 30, 2019 18:48
github-actions bot pushed a commit that referenced this pull request Dec 30, 2019
@ponylang-main
Update CHANGELOG for PR #60 [skip ci]
6cb23d5
@SeanTAllen
Copy link
Member

Merged. After tonight's nightly we should be able to remove the temporary musl script.

@Theodus can you open an issue to remove the ifdefs in the future once they are no longer needed?

@Theodus Theodus mentioned this pull request Dec 30, 2019
Closed
SeanTAllen added a commit that referenced this pull request Dec 31, 2019
@SeanTAllen
Remove temporary musl bootstrap tester
eaed88b 
No longer needed. Was only needed until the code under test
was available as a nightly. See #60
@SeanTAllen SeanTAllen mentioned this pull request Dec 31, 2019
Merged
Theodus pushed a commit that referenced this pull request Dec 31, 2019
@SeanTAllen @Theodus
Remove temporary musl bootstrap tester
6d7bd85 
No longer needed. Was only needed until the code under test
was available as a nightly. See #60
SeanTAllen added a commit that referenced this pull request Dec 31, 2019
@SeanTAllen
Remove temporary musl bootstrap tester
4996ad2 
No longer needed. Was only needed until the code under test
was available as a nightly. See #60
Theodus pushed a commit that referenced this pull request Dec 31, 2019
@SeanTAllen @Theodus
Remove temporary musl bootstrap tester
7c09fd6 
No longer needed. Was only needed until the code under test
was available as a nightly. See #60
SeanTAllen added a commit that referenced this pull request Jan 1, 2020
@SeanTAllen
Remove temporary musl bootstrap tester
f6716f4 
No longer needed. Was only needed until the code under test
was available as a nightly. See #60
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SeanTAllen SeanTAllen Awaiting requested review from SeanTAllen

Assignees
No one assigned
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Theodus @SeanTAllen