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

Always output a tarball from cc_binary to simplify logic. #936

Merged
merged 6 commits into from
Dec 1, 2021
Merged

Always output a tarball from cc_binary to simplify logic. #936

merged 6 commits into from
Dec 1, 2021

Conversation

walkingeyerobot
Copy link
Collaborator

This will change the result of --config=wasm builds that were previously outputting a single file.

Helps prepare for windows support (#814) and removes the need for #929 to add an external python dep.

…hange the result of --config=wasm builds that were previously outputting a single file.
@walkingeyerobot
Copy link
Collaborator Author

cc @trybka

@sbc100
Copy link
Collaborator

sbc100 commented Nov 19, 2021

Its maybe a little sad that --config=wasm a little less directly useful now since users will always need to untar the wasm file. I guess it will push more folks towards using wasm_cc_binaryen?

# If we have more than one output file then create tarball
if len(files) > 1:
# If we have one or more output files then create a tarball
if len(files) >= 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just drop this if/else completely. If you a worried that would loose coverage perhaps just assert(len(files)) here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I would prefer to maintain the check. I don't expect it to be triggered, but we're not losing anything by keeping it, so I figure it's better to keep it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, but then at least re-write it to use early-return style and avoid the indentation:

if not len(files):
  print('emcc.py did not appear to output any known files!')
  sys.exit(1)

.. the rest here without any indentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@walkingeyerobot
Copy link
Collaborator Author

Its maybe a little sad that --config=wasm a little less directly useful now since users will always need to untar the wasm file. I guess it will push more folks towards using wasm_cc_binaryen?

Hopefully it will. I believe the majority of emscripten users are building and using both the .wasm file and the .js wrapper, so this should affect a very small number of users. It's not too bad to generate a genrule that would untar the archive for you if someone is insistent in not using wasm_cc_binary. And if this does generate enough complaints, we can always go back and add the filetype dep.

In short, this simplifies the code, eliminates a potential external dependency, and shouldn't affect very many users.

@trybka
Copy link

trybka commented Nov 19, 2021

cc @trybka

LGTM (with sbc100's suggested tweak)

@walkingeyerobot
Copy link
Collaborator Author

not sure why the test-linux check is failing; what I did shouldn't have any effect there.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 22, 2021

Looks like an unrelated binaryen failure:

root/project/binaryen/main/src/passes/GlobalRefining.cpp: In member function 'virtual void wasm::{anonymous}::GlobalRefining::run(wasm::PassRunner*, wasm::Module*)':
/root/project/binaryen/main/src/passes/GlobalRefining.cpp:57:27: error: unused variable 'func' [-Werror=unused-variable]
     for (auto& [func, info] : analysis.map) 

@walkingeyerobot
Copy link
Collaborator Author

Looks like an unrelated binaryen failure:

Ok, thanks! Let me know if there's anything else I should do to get this merged. Happy to wait and rebase if necessary. :)

@walkingeyerobot
Copy link
Collaborator Author

Looks like this is still failing. Is there something I need to update to get it to pass?

@sbc100
Copy link
Collaborator

sbc100 commented Dec 1, 2021

I think you just need to rebase (or merge), no?

@walkingeyerobot walkingeyerobot enabled auto-merge (squash) December 1, 2021 23:22
@walkingeyerobot
Copy link
Collaborator Author

Thanks! Sorry about that, my git experience is still quite minimal. I just need an approval now please and thanks!

@walkingeyerobot walkingeyerobot merged commit c7305b4 into emscripten-core:main Dec 1, 2021
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