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

Replace target attribute with targets #428

Closed
wants to merge 1 commit into from

Conversation

attilaolah
Copy link
Contributor

This is so that multiple labels could be provided to a toolchain as sources. An example is emcmake, which needs both emscripten and binaryen to run.

Another reason for this is to pass in config files that might be needed for the toolchain. The config files can be passed in as a label and will be available in ${EXT_BUILD_DEPS}/bin, just like any other directory.

@attilaolah attilaolah marked this pull request as ready for review September 17, 2020 00:28
@attilaolah attilaolah changed the title Replace target attribute with targets. Replace target attribute with targets Sep 17, 2020
attilaolah pushed a commit to attilaolah/wasm that referenced this pull request Jan 21, 2021
attilaolah pushed a commit to attilaolah/wasm that referenced this pull request Jan 23, 2021
This includes 75e74567c76fa0dabf4cc5752af3b7cee7689704 and updates for
rebased pull requests:

- bazel-contrib/rules_foreign_cc#427
- bazel-contrib/rules_foreign_cc#428
@attilaolah
Copy link
Contributor Author

Actually, let's hold off with this: it should be possible to use a file group or something similar to get this to work.

@jsharpe
Copy link
Member

jsharpe commented Feb 2, 2021

Actually, let's hold off with this: it should be possible to use a file group or something similar to get this to work.

Yes that's what I was also thinking; the other way is a sh_binary wrapper script which sets up the PATH env before calling out to the target to reference the tools stored in a data attribute on the sh_binary?

Also see the changes I'm making in #437 - enabling use of $(execpath) in the path attribute - does that help?

@attilaolah
Copy link
Contributor Author

Yes, let me revert this back to a draft PR while I try to plumb something together on my side.

@attilaolah attilaolah marked this pull request as draft February 2, 2021 10:31
This is so that multiple labels could be provided to a toolchain as
sources. An example is `emcmake`, which needs both `emscripten` and
`binaryen` to run.

Another reason for this is to pass in config files that might be needed
for the toolchain. The config files can be passed in as a label and will
be available in ${EXT_BUILD_DEPS}/bin, just like any other directory.
@attilaolah
Copy link
Contributor Author

attilaolah commented Feb 5, 2021

I'm trying to get this to work, but I think it won't be possible until #437 is merged.

  • If I pass in a filegroup as the target label, none of the sources of the filegroup are even built.
  • If I pass in a sh_binary rule, it seems to work but then I lose all the runfiles I still don't get the data files of the corresponding sh_binary built for the host.

@attilaolah
Copy link
Contributor Author

I finally managed to work around this, but I basically had to inject all required tools into tools_deps, which is annoying a bit but I have macros for each rule anyway.

It's not great but should work until #437 is done, and so I don't need this change any more.

@attilaolah attilaolah closed this Feb 5, 2021
@attilaolah attilaolah deleted the targets branch February 5, 2021 21:16
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