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

Stop modifying upstream .gitignore file and put build artifacts in debian/build/ #230

Merged
merged 2 commits into from
Jan 19, 2025

Conversation

ottok
Copy link
Contributor

@ottok ottok commented Nov 29, 2024

@@ -299,7 +300,7 @@ func writeDebianRules(dir string, pkgType packageType) error {
fmt.Fprintf(f, "#!/usr/bin/make -f\n")
fmt.Fprintf(f, "\n")
fmt.Fprintf(f, "%%:\n")
fmt.Fprintf(f, "\tdh $@ --builddirectory=_build --buildsystem=golang\n")
fmt.Fprintf(f, "\tdh $@ --builddirectory=debian/build --buildsystem=golang\n")
Copy link
Member

Choose a reason for hiding this comment

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

NACK. There are already too many packages using that path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not an API. There isn't any downside for new packages to use this
clearly cleaner and better way, and old packages are trivial to update, and
testing that the update works correctly is also trivial. The different
gitignore/clean has been used before should not be an argument on its own.
Otherwise no progress could happen, right?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that would be ideal. Posted first version at https://salsa.debian.org/go-team/packages/dh-golang/-/merge_requests/25, perhaps we can continue the review of that change there?

If it is merged, this can be reduced to just drop those modifications as unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tianon Dropping the flag isn't a good idea, since any d/rules in existing packages rely on the build directory being where it is now when dh_auto_configure/dh_auto_build is overridden.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but this is the generator for new packages, not existing packages. For existing packages where that matters, an explicit override is appropriate, and documented appropriately in man dh-golang (so this point matters more for some theoretical automated tool dropping the flag, which could be that it avoids dropping the flag in any case where it isn't set to this value, any case where _build appears elsewhere in the file too, and any case that fails to build after dropping the flag).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the optimal thing to merge both https://salsa.debian.org/go-team/packages/dh-golang/-/merge_requests/25 and this, so that the change is uniform everywhere and everyone gets the same result immediately? And once a new version dh-golang has been out there long enough, we can make dh-make-golang smaller and drop extra stuff from here.

@ottok ottok force-pushed the dont-pollute-top-level-dir branch from d57090d to 5fec732 Compare January 14, 2025 03:20
ottok added 2 commits January 19, 2025 13:52
The fact that Debian builds produce extra files in the build directory
is a separate concern and should not be managed by .gitignores in upstream
directory. Anyways, the list is not going to be complete on most packages,
and instead of extending the list, a better practice is to have proper
`make clean` rules in the `debian/rules`, or to simply run `git clean -fdx`
between builds. Additionally, everyone should be using `gbp pq` to update
patches instead of legacy Quilt, so no more `.pc` directories should be
generated.
…urces

Instead of putting build artifacts in `_build/` at the project root,
where they pollute the upstream sources and lure some people to also
modify the upstream `.gitignore` file, put it in `debian/build/` along
with all other build artifacts that get generated in `debian/`.
@ottok ottok force-pushed the dont-pollute-top-level-dir branch from 5fec732 to 0bdd05c Compare January 19, 2025 21:52
@ottok
Copy link
Contributor Author

ottok commented Jan 19, 2025

As the majority if in favor of this simplification, and it was also accepted and implemented in https://salsa.debian.org/go-team/packages/dh-golang/-/commit/bc16dff5381b668a71fa99c381baba202c34c789, I am now proceeding to merge this.

@ottok ottok merged commit 0bdd05c into Debian:master Jan 19, 2025
2 checks passed
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.

4 participants