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

build: generate buildinfo.txt for test logs #14802

Closed
wants to merge 11 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 5, 2024

Also:

  • read buildinfo.txt from runtests.pl and dump it to the log.
  • cmake: show CROSS target flag for cross-builds.
  • cmake: add logic to detect arguments passed via the command-line.

It is meant to help filling out missing datapoints in the testclutch
matrix.

Also read this from runtests.pl and dump it to the log.
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bagder
Copy link
Member

bagder commented Sep 5, 2024

BTW, a more traditional way to generate this file would be to have a buildinfo.txt.in template like curl-config.in and libcurl.pc.in.

@vszakats
Copy link
Member Author

vszakats commented Sep 5, 2024

BTW, a more traditional way to generate this file would be to have a buildinfo.txt.in template like curl-config.in and libcurl.pc.in.

Yes, considered it. Upside is that the fields would be guaranteed to match between cmake and autotools, but they also use a slightly different set now, so I went for the simpler, looser approach. .in is also more involved with a new set of macro names, etc. ..I'll give it some more thought.

(Converting configurehelp.pm to .in seems like a clear tidy-up candidate though.)

@vszakats
Copy link
Member Author

vszakats commented Sep 6, 2024

There is plenty of tweaking opportunity here:

  • place buildinfo.txt to root instead of tests?
  • perhaps dump buildinfo.txt in CI for jobs that don't run tests? (for debugging build, and debugging detection itself)
  • also dump it early in CI? (Perhaps as part of the config.h dump step)
  • align target flags to have a non-CMake-specific feel (lowercase them for example)
  • try to bring the set of fields and their contents more in sync and consistent across autotools and cmake?
  • tweak log output? (it's a tad verbose now) Perhaps hide it under a GHA log fold?
  • tweak file format, field names?

I'd say it's largely good as it is for a start and we can tweak it as we go.

@vszakats vszakats closed this in 1fdea16 Sep 6, 2024
@vszakats vszakats deleted the buildinfo branch September 6, 2024 19:49
@dfandrich
Copy link
Contributor

I've modified Test Clutch to use these new fields; you should start seeing them tomorrow. I found a couple of inconsistencies I'd love to see addressed, though:

  • buildinfo.target is blank on cmake. If it can't be determined, the line need not be written
  • buildinfo.compiler.version write the human-readable compiler version on CMake but an internal processed version on autoconf (e.g. 12.3.0 vs 1200). It would be nice to have both these be the human-readable versions, as the internal one probably isn't very interesting for anything to see. configure writes both the processed one and the "raw" one, which seems to drop the minor and patch versions. The important thing is for it to be more consistent, so "raw" would be better if there's no alternative.

Some fields are duplicates:

  • buildinfo.host.os is the same as the "OS: " line (but possibly in a different form from a different source)
  • buildinfo.{host,target}.vendor can be derived from buildinfo.{host,target} (when it exists, and it only exists when it is found)
  • buildinfo.configure.os is (presumably) always the same as "OS: " or buildinfo.host.os

@vszakats
Copy link
Member Author

vszakats commented Sep 7, 2024

@dfandrich: Thanks for the testclutch update and your feedback!

I'll see how to include the raw compiler version from autotools.

configure.os is CMake only, and shows the OS version, too. This may be useful in theory. With current jobs and tests, it's probably not overly relevant.

The rest is mostly caused by the different worldviews of the tools. With autotools host/target is always the same as cpu-vendor-os. With CMake, it's a bit all over the place. target is filled in a couple of jobs, but manually via CMAKE_C_COMPILER_TARGET. It seems safe to drop. For autotools, the individual cpu-vendor-os could be dropped in favour for target / host triplets.

vszakats added a commit that referenced this pull request Sep 19, 2024
- cmake: drop `configure.os`.
  This also includes OS version, but thus far it's not important enough
  to include it.
- autotools: drop redundant, autotools-only `{target|host}.vendor`.
  (it's part of the triplet in `{target|host}`.)
- swap order to `*.cpu` -> `*.os` to match triplet-order.
- cmake: drop redundant `target`.
  It's manually filled and only in a (so far) few CI jobs. Let's revisit
  when this becomes useful.
- move `buildinfo.txt` to build root.
- dist: add `buildinfo.txt` to `DISTCLEANFILES`.
- autotools: detect human readable compiler version.
- autotools: replace `XXYY` `compiler.version` with "X.Y"-style.
  (also to match cmake.)
- autotools: use distinct `compiler_id` for Apple clang: `APPLECLANG`.
  To match cmake and also because the the "X.Y"-style version number
  is the Apple version, while `XXYY` was a value roughly translated to
  mainline llvm/clang version.
- show buildinfo at the end of the configure stage, when run in CI, or
  when `CURL_BUILDINFO` or `CURL_CI` env is set.

Follow-up to 1fdea16 #14802
Assisted-by: Dan Fandrich
Ref: #14802 (comment)
Closes #14822
moritzbuhl pushed a commit to moritzbuhl/curl that referenced this pull request Sep 20, 2024
- cmake: drop `configure.os`.
  This also includes OS version, but thus far it's not important enough
  to include it.
- autotools: drop redundant, autotools-only `{target|host}.vendor`.
  (it's part of the triplet in `{target|host}`.)
- swap order to `*.cpu` -> `*.os` to match triplet-order.
- cmake: drop redundant `target`.
  It's manually filled and only in a (so far) few CI jobs. Let's revisit
  when this becomes useful.
- move `buildinfo.txt` to build root.
- dist: add `buildinfo.txt` to `DISTCLEANFILES`.
- autotools: detect human readable compiler version.
- autotools: replace `XXYY` `compiler.version` with "X.Y"-style.
  (also to match cmake.)
- autotools: use distinct `compiler_id` for Apple clang: `APPLECLANG`.
  To match cmake and also because the the "X.Y"-style version number
  is the Apple version, while `XXYY` was a value roughly translated to
  mainline llvm/clang version.
- show buildinfo at the end of the configure stage, when run in CI, or
  when `CURL_BUILDINFO` or `CURL_CI` env is set.

Follow-up to 1fdea16 curl#14802
Assisted-by: Dan Fandrich
Ref: curl#14802 (comment)
Closes curl#14822
vszakats added a commit that referenced this pull request Sep 21, 2024
Before this patch, each build tool generated `tests/configurehelp.pm`
manually.

Ref: #14802 (comment)
Closes #14819
vszakats added a commit to vszakats/curl that referenced this pull request Oct 12, 2024
vszakats added a commit that referenced this pull request Oct 13, 2024
If present.

It aims to provide TextClutch the same build information that
`runtests.pl` already is providing.

Ref: https://testclutch.curl.se/static/reports/feature-matrix.html
Ref: #15256
Follow-up to 1fdea16 #14802
Closes #15279
vszakats added a commit to vszakats/curl that referenced this pull request Nov 6, 2024
It has the side-effect of silencing CMake warnings about unused
variables passed via the command-line.

Drop it till a better method is found to retrieve this.

Reported-by: Kai Pastor
Ref: curl#14936 (comment)
Follow-up to 1fdea16 curl#14802
vszakats added a commit that referenced this pull request Nov 8, 2024
Collecting the args list has the undesired side-effect of silencing
CMake warnings about unused variables passed via the command-line.

Drop it till a better method is found to retrieve them.

Reported-by: Kai Pastor
Ref: #14936 (comment)
Follow-up to 1fdea16 #14802
Closes #15501
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants