Spec URL: https://linkdupont.fedorapeople.org/reviews/golang-github-migrate.spec SRPM URL: https://linkdupont.fedorapeople.org/reviews/golang-github-migrate-4.16.2-1.fc39.src.rpm Description: Go database migrations library and program Fedora Account System Username: linkdupont Fedora Review: https://download.copr.fedorainfracloud.org/results/linkdupont/reviews/fedora-38-x86_64/06126187-golang-github-migrate/fedora-review/
> %global godocs CONTRIBUTING.md FAQ.md GETTING_STARTED.md MIGRATIONS.md README.md SECURITY.md cmd/migrate/README.md database/sqlite3/README.md source/iofs/README.md This line is too long and those README files won't be installed correctly, you need to rename them. Check "golang-github-cloudflare-circl.spec" on how ecc-p384-LICENSE is created for example. > for cmd in cmd/* ; do > BUILDTAGS="%{_gobuildtags}" %gobuild -o %{gobuilddir}/bin/$(basename $cmd) %{goipath}/$cmd > done It may be just a style thing, but I would split the line in a export and the build command: export BUILDTAGS="%{_gobuildtags}" %gobuild -o %{gobuilddir}/bin/migrate %{goipath}/cmd/migrate > %check > # Tests are skipped because they rely on dktest, a container testing harness > # that expects to have access to the Internet as well as a running docker daemon > # on the test host. This also would be style thing, but I found that usually we just add a bcond in the top for check and just leave the regular %check / %gocheck. In golang-github-git-5.spec for example: # Needs network access to Github %bcond_with check (...) %if %{with check} %check %gocheck %endif
I noticed the bug with the docs too. I fixed it, but have had to duplicate the list of docs; once in %godocs and again in %files. I like the %bcond approach. I reworked the spec file to use that logic instead. Spec URL: https://linkdupont.fedorapeople.org/reviews/golang-github-migrate.spec SRPM URL: https://linkdupont.fedorapeople.org/reviews/golang-github-migrate-4.16.2-1.fc39.src.rpm Fedora Review: https://download.copr.fedorainfracloud.org/results/linkdupont/reviews/fedora-38-x86_64/06179347-golang-github-migrate/fedora-review/
Created attachment 1976225 [details] The .spec file difference from Copr build 6126367 to 6179446
> but have had to duplicate the list of docs; once in %godocs and again in %files. Yes, you've to duplicate the list. For "%global godocs" and "%doc"s in the %files section you can have something in between of a long line and a line per file. COL80 is best practice. >> for cmd in cmd/* ; do >> BUILDTAGS="%{_gobuildtags}" %gobuild -o %{gobuilddir}/bin/$(basename $cmd) %{goipath}/$cmd >> done > > It may be just a style thing, but I would split the line in a export and the build command: > > export BUILDTAGS="%{_gobuildtags}" > %gobuild -o %{gobuilddir}/bin/migrate %{goipath}/cmd/migrate Have you checked this? I'm fine if you don't use this way, just wanted to make sure you've checked it. > Name: %{goname} What about chanching the name of the package to "migrate"? You can generate it with "go2rpm --name migrate github.com/golang-migrate/migrate". It will add a new %global entry, you can check examples like `doctl` or `gh`. > %global common_description %{expand: > Go database migrations library and program.} It would be great, but not requried, if you could add in the description which supported DBs are included with the Fedora build. Something like: %global common_description %{expand: Go database migrations library and program. Supports: * file * iofs * sqlite3 * postgres }
(In reply to Mikel Olasagasti Uranga from comment #4) > For "%global godocs" and "%doc"s in the %files section you can have > something in between of a long line and a line per file. COL80 is best > practice. I took a middle-ground approach and combined the package docs into a couple of lines, limited by 80 characters. For the sources and databases, I opted for one-per-line; hopefully this makes adding new sources and databases later a little easier. > > export BUILDTAGS="%{_gobuildtags}" > > %gobuild -o %{gobuilddir}/bin/migrate %{goipath}/cmd/migrate > > Have you checked this? I'm fine if you don't use this way, just wanted to > make sure you've checked it. I did. I think it really comes down to a matter of style. I suppose there is the possibility that exporting the variable would be a better way of ensuring it gets included in the %gobuild execution line, but I can't think of a situation where that nuance would matter. > > Name: %{goname} > > What about chanching the name of the package to "migrate"? > > You can generate it with "go2rpm --name migrate > github.com/golang-migrate/migrate". It will add a new %global entry, you can > check examples like `doctl` or `gh`. Good idea. I regenerated the package with go2rpm and saw that it just adds `%global goname migrate` after %gometa, so I added that change. > > %global common_description %{expand: > > Go database migrations library and program.} > > It would be great, but not requried, if you could add in the description > which supported DBs are included with the Fedora build. Something like: > > %global common_description %{expand: > Go database migrations library and program. > > Supports: > * file > * iofs > * sqlite3 > * postgres > } Good idea. I modified the description. Spec URL: https://linkdupont.fedorapeople.org/reviews/golang-github-migrate.spec SRPM URL: https://linkdupont.fedorapeople.org/reviews/golang-github-migrate-4.16.2-1.fc39.src.rpm
> I took a middle-ground approach and combined the package docs into a couple of lines, limited by 80 characters. For the sources and databases, I opted for one-per-line; hopefully this makes adding new sources and databases later a little easier. Makes sense. > I did. I think it really comes down to a matter of style. Fine. > Good idea. I regenerated the package with go2rpm and saw that it just adds `%global goname migrate` after %gometa, so I added that change. I forgot to mention you need to edit this BZ ticket's title to match the new name. > Good idea. I modified the description. I like how you did. Change the title and I'll approve the package.
Another thing, there are some packages requires by other backends that are available like golang-github-sql-driver-mysql. Do you plan to add them later?
(In reply to Mikel Olasagasti Uranga from comment #7) > Another thing, there are some packages requires by other backends that are > available like golang-github-sql-driver-mysql. Do you plan to add them later? That's my plan, yea. I'm certain there are some other easy backends like that. But I didn't want this initial review to be burdened by a big pile of dependencies.
Sorry, I just realized a minor issue: > %global goipath github.com/golang-migrate/migrate The correct path is github.com/golang-migrate/migrate/v4 https://github.com/golang-migrate/migrate/blob/master/go.mod#L1
(In reply to Mikel Olasagasti Uranga from comment #9) > Sorry, I just realized a minor issue: > > > %global goipath github.com/golang-migrate/migrate > > The correct path is github.com/golang-migrate/migrate/v4 > > https://github.com/golang-migrate/migrate/blob/master/go.mod#L1 Good catch. Corrected. I noticed since setting the %goname to 'migrate', the resulting SRPM is named 'migrate'. Would it make sense to follow that convention and name the dist-git repo and spec file 'migrate' as well?
Yes, didn't realize about that. Spec and and dist-git must match `migrate` name.
* I dind't realize that you did not change the name of the spec.
Renamed, and updated the %goipath. Spec URL: https://linkdupont.fedorapeople.org/reviews/migrate.spec SRPM URL: https://linkdupont.fedorapeople.org/reviews/migrate-4.16.2-1.fc39.src.rpm
Fedora Review: https://download.copr.fedorainfracloud.org/results/linkdupont/reviews/fedora-38-x86_64/06182707-migrate/fedora-review
It seems `_gobuildtags` is not enough: # dnf install https://download.copr.fedorainfracloud.org/results/linkdupont/reviews/fedora-38-x86_64/06182707-migrate/golang-github-migrate-4-devel-4.16.2-1.fc38.noarch.rpm Last metadata expiration check: 1:47:16 ago on 23-07-19 09:27:29. golang-github-migrate-4-devel-4.16.2-1.fc38.noarch.rpm 176 kB/s | 137 kB 00:00 Error: Problem: conflicting requests - nothing provides golang(cloud.google.com/go/spanner/admin/database/apiv1/databasepb) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/dhui/dktest) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/google/go-github/v39/github) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/jackc/pgerrcode) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/jackc/pgx/v5/pgconn) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/jackc/pgx/v5/stdlib) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/ktrysmt/go-bitbucket) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/microsoft/go-mssqldb) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/mutecomm/go-sqlcipher/v4) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/nakagami/firebirdsql) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/neo4j/neo4j-go-driver/neo4j) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(modernc.org/ql/driver) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(modernc.org/sqlite) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline (try to add '--skip-broken' to skip uninstallable packages) Checking the review file you can see in the "Requires" and "Provides" sections that all the DBs are taking into consideration, not only those built with _gobuildtags. https://download.copr.fedorainfracloud.org/results/linkdupont/reviews/fedora-38-x86_64/06182707-migrate/fedora-review/review.txt I think the simplest solution is to `rm` the non built databases in %prep.
You're correct. Using build tags just alters the state of the `migrate` binary, but doesn't change the resulting "gocode" package. I assumed that having source files in the -devel package would be harmless enough, but it appears that they're still used to generate dependencies. I reworked the spec file significantly again. This time I took the "removal approach" as you suggest. I also took the time to include any driver that has dependencies already packaged in Fedora. So the drivers that are removed are the ones that are missing a dependency. It's not as daunting a list as I first assumed, but it's still large enough that I don't want to complicate this review unnecessarily. The good news is many common database and source drivers are now included. Spec URL: https://linkdupont.fedorapeople.org/reviews/migrate.spec SRPM URL: https://linkdupont.fedorapeople.org/reviews/migrate-4.16.2-1.fc39.src.rpm Fedora Review: https://download.copr.fedorainfracloud.org/results/linkdupont/reviews/fedora-38-x86_64/06186335-migrate/fedora-review
Created attachment 1976549 [details] The .spec file difference from Copr build 6182694 to 6186417
It still fails with: # dnf install https://download.copr.fedorainfracloud.org/results/linkdupont/reviews/fedora-38-x86_64/06186335-migrate/golang-github-migrate-4-devel-4.16.2-1.fc38.noarch.rpm Waiting for process with pid 1373971 to finish. Last metadata expiration check: 0:00:04 ago on 23-07-19 17:41:25. golang-github-migrate-4-devel-4.16.2-1.fc38.noarch.rpm 183 kB/s | 115 kB 00:00 Error: Problem: conflicting requests - nothing provides golang(github.com/dhui/dktest) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline - nothing provides golang(github.com/google/go-github/v39/github) needed by golang-github-migrate-4-devel-4.16.2-1.fc38.noarch from @commandline (try to add '--skip-broken' to skip uninstallable packages) For `golang(github.com/google/go-github/v39/github)` you can try to replace to latest as done at golang-cloud-google.spec or gopass.spec. If it doesn't work you can either remove that backend, create package `golang-github-google-go-github-39` or patch the app. For golang(github.com/dhui/dktest) removing the files, test files, should be the easiet. > golang-github-migrate-4-devel.noarch: E: zero-length /usr/share/gocode/src/github.com/golang-migrate/migrate/v4/database/crate/README.md > golang-github-migrate-4-devel.noarch: E: zero-length /usr/share/gocode/src/github.com/golang-migrate/migrate/v4/database/shell/README.md Do not include those files
(In reply to Mikel Olasagasti Uranga from comment #18) > For `golang(github.com/google/go-github/v39/github)` you can try to replace > to latest as done at golang-cloud-google.spec or gopass.spec. If it doesn't > work you can either remove that backend, create package > `golang-github-google-go-github-39` or patch the app. Fixed. This was nicer too. It allowed me to include the github source driver. > For golang(github.com/dhui/dktest) removing the files, test files, should be > the easiet. Fixed. > > golang-github-migrate-4-devel.noarch: E: zero-length /usr/share/gocode/src/github.com/golang-migrate/migrate/v4/database/crate/README.md > > golang-github-migrate-4-devel.noarch: E: zero-length /usr/share/gocode/src/github.com/golang-migrate/migrate/v4/database/shell/README.md > > Do not include those files Fixed. I also noticed a couple issues with the way the 'migrate' program was built. It requires buildtags explicitly; otherwise the driver is not included in the resulting binary. I also added an LDFLAG to set the internal version string to %version.
Fixed up the gitlab build issue by including a patch that updates go-gitlab to 0.81.0. I also submitted the patch upstream. Spec URL: https://linkdupont.fedorapeople.org/reviews/migrate.spec SRPM URL: https://linkdupont.fedorapeople.org/reviews/migrate-4.16.2-1.fc39.src.rpm Fedora Review: https://download.copr.fedorainfracloud.org/results/linkdupont/reviews/fedora-38-x86_64/06199583-migrate/fedora-review/
Created attachment 1976943 [details] The .spec file difference from Copr build 6186417 to 6200299
Spec URL: https://linkdupont.fedorapeople.org/reviews/migrate.spec SRPM URL: https://linkdupont.fedorapeople.org/reviews/migrate-4.16.2-1.fc39.src.rpm Fedora Review: https://download.copr.fedorainfracloud.org/results/linkdupont/reviews/fedora-38-x86_64/06207922-migrate/fedora-review
Thanks for all the changes. This is go2rpm package/spec with some tweaks to adapt to Fedora build's requirements. - [x] The specfile is sane. - [x] License is correct - [x] Builds successfully in mock - [x] Package is installable (checked by fedora-review) - [x] No relevant rpmlint errors - [x] %check section passes - [x] The latest version is packaged - [x] `%goipath` is set correctly - [x] Binaries don't conflict with binaries already in the distribution - [x] The package complies with the Packaging Guidelines. Package approved! On import, don't forget to do the following: - [ ] Add package to release-monitoring.org - [ ] Give go-sig privileges on package - [ ] Close the review bug by referencing it in the rpm changelog and the Bodhi ticket.
The Pagure repository was created at https://src.fedoraproject.org/rpms/migrate
FEDORA-2023-7cce29f715 has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2023-7cce29f715
FEDORA-2023-7cce29f715 has been pushed to the Fedora 38 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-7cce29f715` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-7cce29f715 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2023-7cce29f715 has been pushed to the Fedora 38 stable repository. If problem still persists, please make note of it in this bug report.