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

# is not supported in srcs (duplicate of #374) #2006

Closed
wsxiaoys opened this issue Oct 28, 2016 · 13 comments
Closed

# is not supported in srcs (duplicate of #374) #2006

wsxiaoys opened this issue Oct 28, 2016 · 13 comments

Comments

@wsxiaoys
Copy link

wsxiaoys commented Oct 28, 2016

Hey, I'm trying to add bazel support for gambit-scheme, while it have a convention that using "#" in its scheme header files:
https://github.com/gambit/gambit/tree/master/lib

As these header files will be dependencies at gambit scheme's compile time, there's no easy workaround by simple renaming them.

It looks that a fix can be achieved by adding '#' into following variable.

private static final CharMatcher PUNCTUATION_REQUIRING_QUOTING = CharMatcher.anyOf("+,=~");

Any suggestions? I can send such a fix if that's an acceptable solution.

Thanks
Meng

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Oct 31, 2016

Interesting idea. I added # to that set and ran our tests and everything passes, so from a technical point-of-view I think this would be fine. However given how old Bazel is (inside Google) I'm sure this came up earlier and there's probably a reason why # is not supported in target names.

Adding some people who can hopefully provide insight: @ulfjack, @laurentlb, @lberki

@damienmg
Copy link
Contributor

This the general problem of extending the label syntax to accept arbitrary characters in it. I would say dupe of #374

@laszlocsomor
Copy link
Contributor

Agreed, closing as duplicate.

@laszlocsomor laszlocsomor changed the title # is not supported in srcs # is not supported in srcs (duplicate of #374) Oct 31, 2016
@wsxiaoys
Copy link
Author

wsxiaoys commented Oct 31, 2016

Thanks for referencing #374

It seems the problem is identified and the only blocking issue is that the current bazel base might "assume the label is trival" thus a escaping scheme is required.

While in case of "#", @laszlocsomor pointed out all test cases passed without other modificiation. So is it ok to add "#" as a short-term solution?

@damienmg
Copy link
Contributor

The ultimate problem is that Bazel cannot address a file that has a # in its name, barely fixing the query pattern won't be enough.

@laszlocsomor
Copy link
Contributor

@damienmg , why not? What prevents us from extending the label syntax to accept # in the target name?

@lberki
Copy link
Contributor

lberki commented Oct 31, 2016

I am personally worried about all the Google-internal stuff that assumes that labels are of a specific syntax. I think we should nevertheless change it, but not piecemeal (add #, then add !, then...), but in one go so that these things only need to be updated once.

@wsxiaoys
Copy link
Author

wsxiaoys commented Oct 31, 2016

@lberki Current implementation already allow characters like +-@ and bash-like environment treat '#' no different than them.

So if we add '#' here:

  1. Un-aware checking code still disallow '#': stuff won't break until someone want to use this feature. An "update" would mostly like redirect its checking functionality to LabelValidator.
  2. Un-aware checking code doesn't check for '#': work as is.

In either case seems we don't require more-than-one updates to internal stuff.

@wsxiaoys
Copy link
Author

wsxiaoys commented Nov 2, 2016

Kindly ping on question raised by @laszlocsomor

@wsxiaoys
Copy link
Author

wsxiaoys commented Nov 7, 2016

Ping

@damienmg
Copy link
Contributor

damienmg commented Nov 7, 2016

Changing

public static String validateTargetName(String targetName) {
is needed. And we need to add test for the new character, that will break a lot of service depending on the label name.

I definitely wants to prioritize doing that work, but let's do it 50 times.

@wsxiaoys
Copy link
Author

wsxiaoys commented Nov 7, 2016

As discussed in OP and verified by @laszlocsomor, modification to validateTargetName only involve adding '#' to PUNCTUATION_REQUIRING_QUOTING.

Could you name a few example services would be broken after adding the test? (I assume you're referring some integrating test as unit test is pretty easy to added.)

If it's google internal service, wouldn't it be better to simply leave it as is, given it would simply fall into cases described in #2006 (comment)?

@damienmg
Copy link
Contributor

damienmg commented Nov 7, 2016

If such a simple change work for you, we can always try to run it with the internal tests, if everything pass then it should be pretty safe.

bazel-io pushed a commit that referenced this issue Nov 18, 2016
As suggested by @damienmg in #2006 send this out to run it with internal test.

Closes #2059 .

Progress towards #374.

--
Reviewed-on: #2059
MOS_MIGRATED_REVID=139562084
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants