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

Rename upstream directory to simply prebuilt #1171

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 14, 2023

The name upstream doesn't have any meaning anymore now that there is no more fastcomp. It was removed from the SDK names in #1166 and this is the last usage of that name/term.

@sbc100 sbc100 force-pushed the rename_upstream branch 2 times, most recently from 390d449 to cb87d85 Compare January 14, 2023 19:19
The name `upstream` doesn't have any meaning anymore now that there is
no more fastcomp.  It was removed from the SDK names in #1166 and this
is the last usage of that name/term.
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.

This sounds good, but I think it will surprise some users that have hardcoded paths using upstream/. We never recommended that but they might have just found where things are, and depended on it.

The best I can think of is to add a changelog to this repo, either in the readme or a separate file, and mention this there. We should also mention it in the emscripten changelog (but just mentioning it there doesn't seem like enough, since this will affect users using our standard install mechanism, which uses newest emsdk of the latest released emscripten; one option might be to wait to land this until right before an emscripten tag, but that has its own risks...)

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 17, 2023

This sounds good, but I think it will surprise some users that have hardcoded paths using upstream/. We never recommended that but they might have just found where things are, and depended on it.

The best I can think of is to add a changelog to this repo, either in the readme or a separate file, and mention this there. We should also mention it in the emscripten changelog (but just mentioning it there doesn't seem like enough, since this will affect users using our standard install mechanism, which uses newest emsdk of the latest released emscripten; one option might be to wait to land this until right before an emscripten tag, but that has its own risks...)

Yeah, I was thinking the same thing.. I will hold off on this change for a bit as I know some folks were hardcoding paths to upstream. Ideally nobody would be doing that.. so maybe I'll start by sending out a message to the list to see if we can get folks to stop doing it.

@DreamOfIce
Copy link
Contributor

This sounds good, but I think it will surprise some users that have hardcoded paths using upstream/. We never recommended that but they might have just found where things are, and depended on it.

The best I can think of is to add a changelog to this repo, either in the readme or a separate file, and mention this there. We should also mention it in the emscripten changelog (but just mentioning it there doesn't seem like enough, since this will affect users using our standard install mechanism, which uses newest emsdk of the latest released emscripten; one option might be to wait to land this until right before an emscripten tag, but that has its own risks...)

I think maybe we can create a hard link or a symlink from upstream/ to the new folder. This will encourage downstream updates to new folder names while retaining as much compatibility as possible

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.

3 participants