Skip to content

make: build virtme-ng-init by default #200

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

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Conversation

matttbe
Copy link
Collaborator

@matttbe matttbe commented Dec 3, 2024

As mentioned in the README file, the make command should only build virtme-ng-init. Building is what make usually does, and not installing stuff.

Since commit 85a9b42 ("Make it build on macOS"), make was building virtme-ng-init, but it was also trying to install it with other files elsewhere on the system, what make install does. That's because make was using the first target as the default one, so the install one.

Instead of directly calling cargo like it was the case before, the build part of setup.py is called. This will generate files in the build dir that are probably not needed, but it is quick, and it will also build virtme-ng-init properly, what we want. Cc: @tamird.

While at it:

  • Mention all targets that are not linked to files in .PHONY.

  • Only generate the init file when needed: when the .rs files have been modified.

  • Initialise the submodule if the .rs files are not present.

  • A new clean target can help to force rebuilding (or just to clean, even if it looks like it missed many generated files...).

@tamird
Copy link
Contributor

tamird commented Dec 3, 2024

Since commit 85a9b42 ("Make it build on macOS"), make was building virtme-ng-init, but it was also trying to install it, what make install does.

Are you referring to the use of cargo install? Because that isn't "installing" in the way you're implying because of the --root argument which causes cargo to "install" into the package, not system-wide destination.

I'm not really sure what problem this PR is solving but this description is misleading. Please consider describing the true motivation.

As mentioned in the README file, the 'make' command should only build
virtme-ng-init. Building is what 'make' usually does, and not installing
stuff.

Since commit 85a9b42 ("Make it build on macOS"), 'make' was building
virtme-ng-init, but it was also trying to install it with other files
elsewhere on the system, what 'make install' does. That's because make
was using the first target as the default one, so the 'install' one.

Instead of directly calling 'cargo' like it was the case before, the
'build' part of setup.py is called. This will generate files in the
build dir that are probably not needed, but it is quick, and it will
also build virtme-ng-init properly, what we want.

While at it:

- Mention all targets that are not linked to files in .PHONY.

- Only generate the init file when needed: when the .rs files have been
  modified.

- Initialise the submodule if the .rs files are not present.

- A new 'clean' target can help to force rebuilding (or just to clean,
  even if it looks like it missed many generated files...).

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
@matttbe
Copy link
Collaborator Author

matttbe commented Dec 3, 2024

Since commit 85a9b42 ("Make it build on macOS"), make was building virtme-ng-init, but it was also trying to install it, what make install does.

Are you referring to the use of cargo install? Because that isn't "installing" in the way you're implying because of the --root argument which causes cargo to "install" into the package, not system-wide destination.

I'm not really sure what problem this PR is solving but this description is misleading. Please consider describing the true motivation.

Thanks, I just updated the description: we don't want make to run make install by default, just the build for a local use:

Since commit 85a9b42 ("Make it build on macOS"), make was building virtme-ng-init, but it was also trying to install it with other files elsewhere on the system, what make install does. That's because make was using the first target as the default one, so the install one.

Is it clearer?

@matttbe
Copy link
Collaborator Author

matttbe commented Dec 3, 2024

Note: I used python3 setup.py build, but maybe there is a better command to use?

@arighi
Copy link
Owner

arighi commented Dec 3, 2024

Note: I used python3 setup.py build, but maybe there is a better command to use?

We could detect python3 || python, but I wouldn't over-complicate things here, running python3 setup.py build seems good enough.

@tamird
Copy link
Contributor

tamird commented Dec 3, 2024

Thanks for clarifying.

@matttbe
Copy link
Collaborator Author

matttbe commented Dec 3, 2024

Note: I used python3 setup.py build, but maybe there is a better command to use?

We could detect python3 || python, but I wouldn't over-complicate things here, running python3 setup.py build seems good enough.

Good point. We can indeed change that later. Are there still some distributions not having a python3 binary, only python? I remembered issues a few years ago when using python, but not sure about python3.

@matttbe matttbe merged commit ba08268 into arighi:main Dec 3, 2024
6 checks passed
@arighi
Copy link
Owner

arighi commented Dec 3, 2024

Good point. We can indeed change that later. Are there still some distributions not having a python3 binary, only python? I remembered issues a few years ago when using python, but not sure about python3.

Not that I'm aware of, I think all the recent distro have python3.

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.

3 participants