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

macros.meson: use setup --reconfigure for in-place builds #13340

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keszybz
Copy link
Contributor

@keszybz keszybz commented Jun 19, 2024

The case where the build directory already exists is rare, usually rpm builds are done from scratch. But in case of in-place builds (where the rpm package is built from an already-unpacked development tree) and in case of short-circuit builds where parts of the build are skipped to save time, the directory may exist. Obviously, such solution are not perfect, the build is not done in a pristine environment and the user has to be careful, but users already know this and are willing to take the risk because of the massive time savings.

Without --reconfigure, 'meson setup' says "directory was already configured" and proceeds. Add --reconfigure so that the configuration is applied. If the build directory doesn't exist, this has no effect.

(I initially wanted to do this in systemd, but it seems more reasonable to push it to the macros, since it applies to all rpm packages: systemd/systemd#32321)

@DaanDeMeyer
Copy link
Contributor

This slows down builds as we will now always reconfigure whereas we previously didn't if no options change. I think meson should be fixed to not log a message if options are passed to meson setup.

Also --reconfigure doesn't actually do a proper job anyway as environment variables are not reread so now flags will not apply anyway.

@eli-schwartz
Copy link
Member

I think meson should be fixed to not log a message if options are passed to meson setup.

What do you mean? We should silently do nothing instead of loudly doing nothing?

Also --reconfigure doesn't actually do a proper job anyway as environment variables are not reread so now flags will not apply anyway.

Indeed. If you want to reapply all project setup then you have to wipe the build directory and possibly use ccache.

@DaanDeMeyer
Copy link
Contributor

I think meson should be fixed to not log a message if options are passed to meson setup.

What do you mean? We should silently do nothing instead of loudly doing nothing?

Right now we loudly log and then do something. It makes users think that they're doing something wrong even though running meson setup on an already configured build directory is supported now. So I don't see the point in complaining about users doing it tbh.

@keszybz
Copy link
Contributor Author

keszybz commented Jun 19, 2024

Hmm, indeed. I misunderstood what meson setup is doing:

$ meson configure build|grep remote           
  remote                         disabled                         [enabled, disabled, auto]        support for "journal over the network"                       
$ meson setup build -Dremote=enabled 
Directory already configured.

Just run your build command (e.g. ninja) and Meson will regenerate as necessary.
Run "meson setup --reconfigure to force Meson to regenerate.

If build failures persist, run "meson setup --wipe" to rebuild from scratch
using the same options as passed when configuring the build.
$ meson configure build|grep remote 
  remote                         enabled                          [enabled, disabled, auto]        support for "journal over the network"                       

So it looks like a refusal, but actually the changes are applied. So yeah, I agree with Daan that the best option would be drop this verbose message. Instead, meson setup could say something like "Directory was already configured, applying specified changes.".

@keszybz
Copy link
Contributor Author

keszybz commented Jun 27, 2024

Updated to just print one-line message and continue.

Comment on lines +166 to +168
print('Directory already configured, applying new settings.')
else:
print('Directory already configured.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why drop the mconf.run_impl() call? Shouldn't this change be just about changing the logging but keeping the implementation the same?

@keszybz
Copy link
Contributor Author

keszybz commented Jun 28, 2024 via email

Current behaviour was confusing. It *looked* like meson refused the
operation, but in fact it was applying the configuration changes:

Example from systemd repo:

  $ meson configure build|grep remote
    remote          disabled      [enabled, disabled, auto]   support for "journal over the network"
  $ meson setup build -Dremote=enabled
  Directory already configured.

  Just run your build command (e.g. ninja) and Meson will regenerate as necessary.
  Run "meson setup --reconfigure to force Meson to regenerate.

  If build failures persist, run "meson setup --wipe" to rebuild from scratch
  using the same options as passed when configuring the build.
  $ meson configure build|grep remote
    remote           enabled      [enabled, disabled, auto]    support for "journal over the network"

mconf.run_impl() sets the options and returns 0. By removing the special
code that was shoehorned into validate_dirs(), we let the build continue.
We now print a short message to explain what is happening.

This fixes the unexpected behaviour where the configuration options would
be changed, but the setup wouldn't be done. There is no good reason for
'mkosi setup' to not set up the directory. Either it should refuse completely
or work fully. The latter seems more useful and is closer to current behaviour,
so let's do that.

(Inspired by systemd/systemd#32321.)
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.

None yet

3 participants