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

native.repository_name() and ctx.workspace_name() are similar function but with different name #4092

Open
damienmg opened this issue Nov 14, 2017 · 11 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request

Comments

@damienmg
Copy link
Contributor

To get the workspace name of the call site, we do native.repository_name() in a macro and ctx.workspace_name() in a rule. The latter is behaving differently in the main repository probably (returning the name, whereas the repository name can be empty).

This is a confusing API and it needs to be cleaned.

@haberman
Copy link
Contributor

haberman commented Nov 2, 2018

I am also finding it confusing that native.repository_name() returns @ for the main repository. Files in runfiles are scoped to the workspace name, but there doesn't appear to be any way to get this workspace name from a macro. So how can I refer to a runfile without copy/pasting the name of the workspace from WORKSPACE?

@mpictor
Copy link

mpictor commented Nov 21, 2018

I'd also find it very useful to be able to get the "real" workspace name inside a macro, rather than having to specify it.

Use case: I have a lot of include paths that include the workspace/repo name, so that it's always clear from looking at an #include statement exactly what header is being referenced. Currently I have to pass the name of the repo to my macro each time so it can do hdrs=glob(['pub/'+reponame+'/'+name+'/*.h'); if I could reliably determine the workspace name, that'd mean less typing and more that could be determined automagically.


Related issue, visible with multiple repos:
Assume two repos A and B, and B's WORKSPACE has

local_repository(
    name='anotherNameForA',
    path='../A'
)

If we're in B and run bazel, any targets in A that call native.repository_name() will see the repo name is anotherNameForA regardless of the name declared in repo A's WORKSPACE.

IMO the workspace name should always be the name declared in its workspace file, regardless of how it's named when imported by local_repository rules in another workspace.

At least, there should be some way of retrieving the "real" name even if there is a legit case for altering what native.repository_name()/ctx.workspace_name() report.

@jcpetruzza
Copy link

I have the same issue with native.repository_name() returning @. I want to have a macro that generates wrapper-scripts for binary targets, and this requires the binary to be a data dependency of the script. So in order to use rlocation I need the repository name. I've hard-coded it for now, but that makes the macro non-portable.

Incidentally, the title of the issue makes this difficult to find. I spent a while trying to figure out if I was using native.repository_name() wrongly or what :)

@dslomov dslomov added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request and removed category: extensibility > external repositories labels Mar 21, 2019
@xingao267
Copy link
Member

+1 for being able to get the real workspace name from a macro.

@aehlig aehlig removed their assignment Feb 1, 2020
@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
@dgp1130
Copy link
Contributor

dgp1130 commented Jun 27, 2021

Ran into this problem myself today (dgp1130/rules_prerender@cc89f55) and had to make some awkward compromises to my desired API to make it work. Are there any objections to adding a new native.workspace_name() function which works identically to ctx.workspace_name() (just returns the name of the current package's workspace)? That would hopefully reduce confusion about the naming discrepancies and allow macros access to the workspace name.

I'm open to making a PR for this if I can at least confirm that this is right path forward. Seems like we just need to add a new workspaceName() method alongside the existing repositoryName() and update the interface accordingly. ctx.workspace_name() seems to get its data from the package, I imagine it's probably not too hard to do the same here. I'm not very familiar with Bazel internals, but hopefully that's all it takes here?

dgp1130 referenced this issue in dgp1130/rules_prerender Jul 24, 2021
Refs #28.

This introduces a new `prerender_resources()` macro, which is a lower-level of abstraction than `prerender_pages_unbundled()`. It is responsible for simply generating an executing the renderer and collecting its output in a directory. It does **not** perform any subsequent processing like extracting annotations or generating scripts or styles.

As a result, this has no hard or implicit expectation that user code generates HTML files, making this a good candidate for scripts which generate raw data instead of actual web pages.

One consequence of this is that it does *not* make sense to depend on `prerender_component()` targets, because the output is not a web page and will not be bundled. This means that `prerender_resources()` simply takes a `ts_library()` (or any JavaScript files) as inputs, giving the macro less responsibility. The downside however is that we need to replicate the `entry_point` behavior of `nodejs_binary()`, which is quite involved and I don't want to match that exactly.

Instead, `prerender_resources()` requires the full workspace-relative path to be provided. This avoids ambiguity, but is a little annoying and easy to screw up. I did try to require the workspace itself in the path, but this would break `prerender_pages_unbundled()` because there is no way to look up the workspace name in a macro. For now, `prerender_resources()` just doesn't support source files in a different workspace, requiring users to manually copy them over.
@philwo philwo removed the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Nov 29, 2021
dgp1130 added a commit to dgp1130/rules_prerender that referenced this issue Apr 15, 2022
[Migration guide](https://github.com/bazelbuild/rules_nodejs/wiki/Migrating-to-5.0) and [release notes](https://github.com/bazelbuild/rules_nodejs/releases/5.0.0) for the major version change.

`@bazel/*` deps need to be kept on the same major version as `rules_nodejs`, so they were updated at the same time.

Breaking changes which needed to be fixed included:
1. Swapped `node_repositories()` with `nodejs_register_toolchains()` (not really a breaking change and kept NodeJS version the same).
2. `rules_nodejs@5.0.0` sets `exports_directories_only = True` by default, [which breaks `ts_library()`](https://github.com/bazelbuild/rules_nodejs/wiki/Migrating-to-5.0#exports_directories_only). For now, I overrode this to be `exports_directories_only = False` to stay on the old behavior. Most likely upgrading to `ts_project()` will unblock this flag flip.
3. `ts_library()` was moved to `@npm//@bazel/concatjs:index.bzl`, so all the existing `load()` statements had to be updated.
4. `concatjs_devserver()` now seems to require the workspace name as a prefix to `additional_root_paths`. Unfortunately [there is no way to get the workspace name at loading time](bazelbuild/bazel#4092 (comment)) without hard-coding it which isn't feasible for `web_resources_devserver()` which needs to be usable outside the `rules_prerender` workspace. Fortunately `__main__` seems to work (default workspace name) even though this workspace has a custom name. I also tested on `ref/external` and confirmed that a separate workspace is able to load and use `web_resources_devserver()` as expected.
@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label May 24, 2023
@dgp1130
Copy link
Contributor

dgp1130 commented May 24, 2023

My offer to contribute something here still stands, if anyone can confirm that is indeed the right approach to solve this particular problem and that the contribution would be accepted.

@fmeum
Copy link
Collaborator

fmeum commented May 24, 2023

A number of things have changed since then:

  1. When using existing rules (such as in a macro) that support location expansion, $(rlocationpath ...) has been introduced and provides the correct runfiles path without the need to prepend a workspace name.
  2. With --enable_bzlmod, the runfiles directory corresponding to the main repository is always called _main, so in the long-term, ctx.workspace_name() will always return that fixed value and probably go away at some point.
  3. Also with --enable_bzlmod, there is native.module_name(), which returns the name of the Bazel module defining the current repository.

I would suggest the following "migration guide":

  • If you construct runfiles paths in a macro or when using an existing rule, use $(rlocationpath ...) instead.
  • If you construct runfiles paths in a custom rule, use file.short_path[len("../):] if file.short_path.startswith("../") else ctx.workspace_name + "/" + file.short_path.
  • If you construct labels, use native.repository_name() - the repository name of the main repository is the empty string, so this will always return the correct value for labels.
  • If you need some human-readable string identifying the current "context", consider using native.module_name() instead once Bzlmod is enabled by default or use something like native.repository_name() or "<main repository>".

CC @Wyverald

@github-actions github-actions bot removed the stale Issues or PRs that are stale (no activity for 30 days) label May 25, 2023
@dgp1130
Copy link
Contributor

dgp1130 commented Jul 31, 2023

Thanks for the recommendations @fmeum. I tried applying this to my usage, but don't see a clear path to it. Specifically your point:

If you construct runfiles paths, use $(rlocationpath ...) instead.

Are there any suggestions for how to handle runfiles paths in APIs where Make variable expansion is not happening? My particular usage of workspace_name is in ctx.actions.write (here) which does not include Make expansion, meaning $(rlocationpath ...) isn't viable.

I suppose I could refactor that into a genrule, though I typically avoid that when generating files because its platform specific and escaping text gets complicated, especially when user input is included in the generated file. That feels like it's moving backwards from a code maintenance perspective. Would the recommended path be "Use an API which includes Make variable expansion" or is there another approach for such situations?

@fmeum
Copy link
Collaborator

fmeum commented Jul 31, 2023

I updated my comment above to clarify that this only applies when using an existing rule, e.g. in a macro.

When you write your own custom rule, you do have access to ctx.workspace_name, which returns the name of the runfiles directory of the main repository. Your linked approach looks fine to me, I added it to my comment. Why do you need an equivalent of $(rlocationpath ...) in this case?

@dgp1130
Copy link
Contributor

dgp1130 commented Aug 1, 2023

@fmeum, sorry I think I've forgotten a lot of the context here. I remember my original motivation was related to the change in dgp1130/rules_prerender@cc89f55, something about how entry_point needed to be limited to files within the same workspace as the prerender_resources target as specified in the rule docs, but I don't remember exactly how that was fixed by getting workspace names in macros. That API has since been updated so entry_point is always a relative path, so ../../../some_other_wksp/path/to/pkg/file.mjs should work (not that I'd recommend it).

Looking through my existing usages of ctx.workspace_name the only remaining isthis which should be fine because it always points to a file generated in the same rule. I think you're right that ctx.workspace_name is fine there.

Apologies for the noise. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request
Projects
None yet
Development

No branches or pull requests