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

xml test output: remove legacy fallback #24606

Closed
wants to merge 1 commit into from

Conversation

rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Dec 7, 2024

Boost test when writing XML files may sometimes omit the decimal point,
but still write in scientific notation as floating point seconds. To
support reading these XML files, remove the legacy fallback for these time values
where it falls back to integer milliseconds if there are no decimal points.

Fixes: #24605

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Dec 7, 2024
@RaHorEl
Copy link

RaHorEl commented Dec 7, 2024

✍🏾

RaHorEl

This comment was marked as spam.

@travisdowns
Copy link

travisdowns commented Dec 7, 2024

// This is ugly. For Historical Reasons, we have to check whether the number
// contains a decimal point or not. If it does, the number is expressed in [seconds else ms]

Hmm, pretty sure boost doesn't play this game either, e.g., if the time happens to be an integer value like 1 there will be no decimal point in the output. H/e we can work around this on the boost side too.

@rockwotj
Copy link
Contributor Author

rockwotj commented Dec 7, 2024

Yes from everywhere I have read this is supposed to always represent seconds - so maybe it's possible to just remove the millisecond fallback case?

@rockwotj
Copy link
Contributor Author

rockwotj commented Dec 8, 2024

Actually I wonder if the better route is to just remove the millisecond fallback entirely. Every tool that can write this that I have checked uses seconds for their attribute, the code hasn't changed since it was open sourced in 2015. I guess this would require an incompatible change flag. I can update the PR monday

@fmeum
Copy link
Collaborator

fmeum commented Dec 8, 2024

@meisterT Can you check whether Google still relies on the fallback logic?

@meisterT
Copy link
Member

meisterT commented Dec 9, 2024

I think it's obsolete, but I will do some follow-up code archeology. Give me two more days.

Boost test when writing XML files may sometimes omit the decimal point,
but still write in scientific notation as floating point seconds. To
support reading these XML files, if the string contains an `e`, then
try to parse as a float in hopes it's a floating point number. Since
parseLong will not support a `e` character this seems like a safe change
that is unlikely to break anything.

Note that the comment here is fixed as it was phrased opposite of the
actual implementation.
@rockwotj rockwotj force-pushed the scientific-notation branch from fb93455 to 26062d9 Compare December 9, 2024 19:13
@rockwotj rockwotj changed the title xml test output: support scientific notation without decimal xml test output: remove legacy fallback Dec 9, 2024
@rockwotj
Copy link
Contributor Author

rockwotj commented Dec 9, 2024

Thanks @meisterT! I've proactively updated my PR in hopes the code archeology provides to confirm the fallback behavior is obsolete.

@meisterT
Copy link
Member

Spoke too soon, sorry. Turns out it is still necessary and in more than one place :-/

The need was introduced in 2008, removed in 2009 and re-introduced in a couple of places (cargo culting I assume) in 2016+. It is non-trivial to clean these up. While I will see whether I can do that, it will take a while and Bazel for now shouldn't remove the fallback. Sorry again.

@rockwotj
Copy link
Contributor Author

Can we add an --incompatible flag to disable them for Bazel but Blaze can keep them on?

@iancha1992 iancha1992 added the team-Core Skyframe, bazel query, BEP, options parsing, bazelrc label Dec 10, 2024
@meisterT
Copy link
Member

We could, but I am not sure whether it's worth the effort.

@rockwotj rockwotj closed this Dec 11, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Core Skyframe, bazel query, BEP, options parsing, bazelrc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bazel does not accept certain time values on testcases
6 participants