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

cargo_build_script: Add regression test for revert #2925 #2936

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vitalybuka
Copy link
Contributor

@vitalybuka vitalybuka commented Oct 14, 2024

#2931 added tests for _pwd_flags, however
the test as-is passes with #2911 and #2922,
which still caused problems and reverted.
Here I introduce a test case for likely
unfixed in reverted #2925.

The test needs strictter check for cflags. As-ls
flag in cflags is a just substring search and
very weak test for abs path.

For #2917

@vitalybuka
Copy link
Contributor Author

@krasimirgg

@vitalybuka vitalybuka changed the title cargo_build_script: Split CFLASG in test cargo_build_script: Add regression for revert #2925 Oct 14, 2024
@vitalybuka
Copy link
Contributor Author

@UebelAndre

@vitalybuka vitalybuka marked this pull request as draft October 14, 2024 20:59
@vitalybuka
Copy link
Contributor Author

vitalybuka commented Oct 14, 2024

Please review @krasimirgg @UebelAndre
But to avoid conflicts with #2935 marking this as a draft.
I would appreciate if we land #2935 and I rebase this one,
and then upload refactoring to avoid code duplication,
which contained the cause of the revert.

I don't see what can go wrong with #2935, but
downstream, we are blocked on its functionality.

@krasimirgg
Copy link
Collaborator

Could you try to re-format the BUILD.bazel file per the Buildifier message:

##### :bazel: buildifier: found 1 format issue in your WORKSPACE, BUILD and *.bzl files
--
  | 2024-10-16 12:43:50 CEST | If this repo uses a pre-commit hook, then you should install it. Otherwise, please download <a href="https://github.com/bazelbuild/buildtools/releases/tag/v7.3.1">buildifier 7.3.1</a> and run the following command in your workspace:<br/><pre><code>buildifier test/cargo_build_script/cc_args_and_env/BUILD.bazel</code></pre>

@vitalybuka vitalybuka changed the title cargo_build_script: Add regression for revert #2925 cargo_build_script: Add regression test for revert #2925 Oct 16, 2024
As `flag in cflags` is a just substring search
and very weak test for abs path.
Existing test passes with bazelbuild#2911 and bazelbuild#2922.
However even bazelbuild#2922 was breaking something.
I suspect case like this is the cause.
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