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

Move closer to the standard autotools idiom. #3187

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

dag-erling
Copy link
Contributor

  • Create src/config.h instead of passing everything on the compiler command line.

  • To reduce the magnitude of this change, add --include src/config.h to CFLAGS instead of adding #include "src/config.h" at the top of each source file. Not all compilers support this, but I don't think we care about anything other than gcc and clang at this point.

  • Rather than generate src/config_opts.inc, emit JQ_CONFIG directly into src/config.h.

  • Rather than generate src/version.h, define JQ_VERSION as an alias for the standard PACKAGE_VERSION.

* Create `src/config.h` instead of passing everything on the compiler
  command line.

* To reduce the magnitude of this change, add `--include src/config.h`
  to CFLAGS instead of adding `#include "src/config.h"` at the top of
  each source file.  Not all compilers support this, but I don't think
  we care about anything other than gcc and clang at this point.

* Rather than generate `src/config_opts.inc`, emit `JQ_CONFIG` directly
  into `src/config.h`.

* Rather than generate `src/version.h`, define `JQ_VERSION` as an alias
  for the standard `PACKAGE_VERSION`.
@itchyny itchyny added the build label Jan 25, 2025
Copy link
Contributor

@itchyny itchyny left a comment

Choose a reason for hiding this comment

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

This is a nice improvement, thank you.

@itchyny itchyny added this to the 1.8 release milestone Jan 25, 2025
@itchyny itchyny merged commit d8463c2 into jqlang:master Jan 25, 2025
28 checks passed
@itchyny
Copy link
Contributor

itchyny commented Jan 26, 2025

@dag-erling
Sorry, I didn't check the behavior changes of updating version.h before merging. The version string should be followed by the git commit hash, or -dirty suffix if any of the source code is changed. So if I checkout different branch and then make, (or even if I edit one line of main.c and then make), the version string should change. But after merging your PR, it seems that the version string is cached and even ./configure doesn't change the version. This is critical for maintainers because we can't trust the version string of the issue reports. Can you fix this problem?

@itchyny
Copy link
Contributor

itchyny commented Jan 26, 2025

Can we use CONFIGURE_DEPENDENCIES?

@itchyny
Copy link
Contributor

itchyny commented Jan 26, 2025

Hmm, I configure as CONFIG_STATUS_DEPENDENCIES = $(SOURCES), but PACKAGE_VERSION is cached in autom4te.cache/ and the version is not re-evaluated. The package version in AC_INIT is fairly static, and we can't easily allow make to update the version when the git revision (or dirty state) has changed. I understand why we had special rules to make version.h, and regret not confirming well before merging this PR, but let's wait for the author's comment.

itchyny added a commit to itchyny/jq that referenced this pull request Feb 15, 2025
This partially reverts commit d8463c2.
Revert reason: version string should be updated more often.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants