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

Disable fastcomp installation for sdk versions 2.0.0 and above #581

Closed
wants to merge 4 commits into from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Aug 5, 2020

@sbc100 sbc100 requested a review from kripken August 5, 2020 02:40
@@ -2672,6 +2672,9 @@ def expand_sdk_name(name):
backend = 'upstream'
else:
backend = 'fastcomp'
if backend == 'fastcomp' and version_key(fullname) >= (2, 0, 0):
print('Note: fastcomp is not longer available in version 2.0.0 and above')
return name
Copy link
Member

Choose a reason for hiding this comment

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

it might be safer to error here? both code-wise it would be clearer that everything halts here. and the error for users would be better I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The error this produces seems pretty good to me:

$ ./emsdk install 2.0.0-fastcomp
Note: fastcomp is not longer available in version 2.0.0 and above.
Error: No tool or SDK found by name '2.0.0-fastcomp'.

Basically we are failing expand the input given by the user and then the calling code displays the same error the would see if they had made any other kind of typo or something.

Copy link
Member

Choose a reason for hiding this comment

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

I don't love the fact that it depends on 2.0.0-fastcomp not existing and being errored on elsewhere, when we know this should fail right here. But that's a reasonable error.

@@ -2672,6 +2672,9 @@ def expand_sdk_name(name):
backend = 'upstream'
else:
backend = 'fastcomp'
if backend == 'fastcomp' and version_key(fullname) >= (2, 0, 0):
print('Note: fastcomp is not longer available in version 2.0.0 and above')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print('Note: fastcomp is not longer available in version 2.0.0 and above')
print('Note: fastcomp is not longer available in version 2.0.0 and above. please use either the upstream backend, or an earlier fastcomp version')

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm with a test

Base automatically changed from refactor to master August 5, 2020 17:03
@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 7, 2020

Closing in favor of #584

@sbc100 sbc100 closed this Aug 7, 2020
@sbc100 sbc100 deleted the no_fastcomp_after_v2 branch March 25, 2021 15:23
vargaz pushed a commit to vargaz/emsdk that referenced this pull request Nov 22, 2023
emscripten-core#581)

* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20231030.1

Microsoft.SourceBuild.Intermediate.source-build-reference-packages
 From Version 9.0.0-alpha.1.23527.1 -> To Version 9.0.0-alpha.1.23530.1

* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20231030.1

Microsoft.SourceBuild.Intermediate.source-build-reference-packages
 From Version 9.0.0-alpha.1.23527.1 -> To Version 9.0.0-alpha.1.23530.1

* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20231101.1

Microsoft.SourceBuild.Intermediate.source-build-reference-packages
 From Version 9.0.0-alpha.1.23527.1 -> To Version 9.0.0-alpha.1.23551.1

* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20231102.1

Microsoft.SourceBuild.Intermediate.source-build-reference-packages
 From Version 9.0.0-alpha.1.23527.1 -> To Version 9.0.0-alpha.1.23552.1

* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20231102.1

Microsoft.SourceBuild.Intermediate.source-build-reference-packages
 From Version 9.0.0-alpha.1.23527.1 -> To Version 9.0.0-alpha.1.23552.1

---------

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
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.

2 participants