Bug 267290 - devel/git-delta: Fix building on i386
Summary: devel/git-delta: Fix building on i386
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Nuno Teixeira
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-23 12:30 UTC by Jesús Daniel Colmenares Oviedo
Modified: 2022-10-31 18:02 UTC (History)
2 users (show)

See Also:


Attachments
git-delta-0.14.0.patch (1.48 KB, patch)
2022-10-23 12:30 UTC, Jesús Daniel Colmenares Oviedo
no flags Details | Diff
git-delta-0.14.0 (1003 bytes, patch)
2022-10-31 02:41 UTC, Jesús Daniel Colmenares Oviedo
DtxdF: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesús Daniel Colmenares Oviedo 2022-10-23 12:30:14 UTC
Created attachment 237557 [details]
git-delta-0.14.0.patch

Description:

Running `/wrkdirs/usr/ports/devel/git-delta/work/target/release/build/onig_sys-181d0300c100d496/build-script-build`
The following warnings were emitted during compilation:

warning: c/freebsd.c:31:10: error: conflicting types for 'get_cpu_speed'
warning: uint64_t get_cpu_speed(void) {
warning:          ^
warning: c/info.h:31:15: note: previous declaration is here
warning: unsigned long get_cpu_speed(void);
warning:               ^
warning: c/freebsd.c:47:10: error: conflicting types for 'get_proc_total'
warning: uint64_t get_proc_total(void) {
warning:          ^
warning: c/info.h:34:15: note: previous declaration is here
warning: unsigned long get_proc_total(void);
warning:               ^
warning: 2 errors generated.

error: failed to run custom build command for `sys-info v0.9.1`

QA:

* portlint: OK (looks fine.)
* testport: OK (poudriere: 13.1-RELEASE, amd64, BASH FISH ZSH tested)

Notes:

* pkg-fallout informed me [1].

[1] https://lists.freebsd.org/archives/freebsd-pkg-fallout/2022-October/283255.html
Comment 1 Alexey Dokuchaev freebsd_committer freebsd_triage 2022-10-24 07:38:58 UTC
Rather than marking the port as BROKEN, would it be possible to fix the type conflict instead?  Returning uint64_t from these functions looks bogus, CPU count and speed are naturally not *that* huge and would hardly ever be.
Comment 3 Nuno Teixeira freebsd_committer freebsd_triage 2022-10-29 00:39:25 UTC
(In reply to Nuno Teixeira from comment #2)
(...)

This sys-info problem was reported in Jan 5, 2021 and no answer yet.
Maybe upstream doesn't support 32 bits at all.
Comment 4 Alexey Dokuchaev freebsd_committer freebsd_triage 2022-10-29 02:31:03 UTC
(In reply to Nuno Teixeira from comment #3)
> Maybe upstream doesn't support 32 bits at all.
It's not about 32 bits, I'd say they're using the wrong types that work elsewhere by incident.
Comment 5 Nuno Teixeira freebsd_committer freebsd_triage 2022-10-29 12:45:41 UTC
(In reply to Alexey Dokuchaev from comment #4)
Any clue on how to fix it or reformulate the problem so it can be added to current issue with the hope that upstream fix it?

Thanks
Comment 6 Alexey Dokuchaev freebsd_committer freebsd_triage 2022-10-29 14:02:18 UTC
(In reply to Nuno Teixeira from comment #5)
> Any clue on how to fix it?
From what I can see, sys-info-0.9.1 crate is extracted into the port's working directory, so fixing it should be as easy as simply patch relevant files (freebsd.c, info.h, lib.rs) to replace uint64_t, unsigned long, u64 in problematic functions with uint_32t and u32 (my preferred version because I don't like to use huge types for small numbers) or just replace "unsigned long" (which width is machine-dependent) with uint64_t if one prefers to stay within 64-bit domain.  The latter would also yield smaller a patch (as only C code would need patching) and perhaps even just one sed(1) call would suffice.  Try this:

post-patch:
	@${REINPLACE_CMD} -e '/get_cpu/s,unsigned long,uint64_t,' \
		${WRKSRC}/cargo-crates/sys-info-0.9.1/c/*.[ch]
Comment 7 Nuno Teixeira freebsd_committer freebsd_triage 2022-10-29 15:23:00 UTC
(In reply to Alexey Dokuchaev from comment #6)

Good news!

'/get_cpu/s,unsigned long,uint64_t,' for 'get_cpu' reduced error to:

---
warning: c/freebsd.c:47:10: error: conflicting types for 'get_proc_total'
warning: uint64_t get_proc_total(void) {
warning:          ^
warning: c/info.h:34:15: note: previous declaration is here
warning: unsigned long get_proc_total(void);
warning:               ^
warning: 1 error generated.
---

I added '/get_proc_total/s,unsigned long,uint64_t,' for 'get_proc_total':

and it builds ok.

reinplace_warnings.txt says that both freebsd.c and test.c weren't touched:
---
- - REINPLACE_CMD ran, but did not modify file contents: cargo-crates/sys-info-0.9.1/c/freebsd.c
- - REINPLACE_CMD ran, but did not modify file contents: cargo-crates/sys-info-0.9.1/c/test.c
- - REINPLACE_CMD ran, but did not modify file contents: cargo-crates/sys-info-0.9.1/c/freebsd.c
- - REINPLACE_CMD ran, but did not modify file contents: cargo-crates/sys-info-0.9.1/c/test.c
---

Need to test devel/git-delta and sysutils/topgrade inside a i386 jail to do a test run.

Summary: Add:
---
post-patch:
        @${REINPLACE_CMD} -e '/get_cpu/s,unsigned long,uint64_t,' \
                ${WRKSRC}/cargo-crates/sys-info-0.9.1/c/*.[ch]
        @${REINPLACE_CMD} -e '/get_proc_total/s,unsigned long,uint64_t,' \
                ${WRKSRC}/cargo-crates/sys-info-0.9.1/c/*.[ch]
---

Be right back with test run results :)
Comment 8 Nuno Teixeira freebsd_committer freebsd_triage 2022-10-29 17:29:28 UTC
(In reply to Nuno Teixeira from comment #7)
(...)

devel/git-delta and sysutils/topgrade:

Tests run OK on FreeBSD 12.3 i386 (bhyve)

sys-info v0.9.1 crate workaround fix:

---
post-patch:
    @${REINPLACE_CMD} \
        -e '/get_cpu/s|unsigned long|uint64_t|' \
        -e '/get_proc_total/s|unsigned long|uint64_t|' \
        ${WRKSRC}/cargo-crates/sys-info-0.9.1/c/*.[ch]
---
Comment 9 Jesús Daniel Colmenares Oviedo 2022-10-31 02:41:23 UTC
Created attachment 237741 [details]
git-delta-0.14.0

Description:

* Change unsigned long to uint64_t on get_cpu and get_proc_total

QA:

* portlint: OK (looks fine.)
* testport: (poudriere: 13.1-RELEASE/amd64 14.0-CURRENT/i386):
  - REINPLACE_CMD ran, but did not modify file contents: cargo-crates/sys-info-0.9.1/c/freebsd.c
  - REINPLACE_CMD ran, but did not modify file contents: cargo-crates/sys-info-0.9.1/c/test.c
Comment 10 Jesús Daniel Colmenares Oviedo 2022-10-31 02:42:34 UTC
Sorry for responding too late.

Thanks for providing the patches to fix the problem on i386.
Comment 11 Nuno Teixeira freebsd_committer freebsd_triage 2022-10-31 12:14:53 UTC
(In reply to Jesús Daniel Colmenares Oviedo from comment #10)

All we have is an workaround to fix it.
Still waiting for upstream reaction.
Comment 12 Alexey Dokuchaev freebsd_committer freebsd_triage 2022-10-31 12:28:48 UTC
(In reply to Nuno Teixeira from comment #11)
> All we have is an workaround to fix it.
It is not a workaround, it's a real fix.  Using particular 32 vs 64-bit types is debatable, but is not essential.

> Still waiting for upstream reaction.
There's no need to wait for anything, you can unbreak the port and move on.  Shall upstream wake up and decides to employ a different fix, you can always backport it later.
Comment 13 Nuno Teixeira freebsd_committer freebsd_triage 2022-10-31 12:39:24 UTC
(In reply to Alexey Dokuchaev from comment #12)

Ok. Just one question:

Should this fix applied to all archs? Or should it be applied only to specified ones like i386?

Thanks
Comment 14 Alexey Dokuchaev freebsd_committer freebsd_triage 2022-10-31 12:44:54 UTC
(In reply to Nuno Teixeira from comment #13)
> Should this fix applied to all archs?
Yes.

> Or should it be applied only to specified ones like i386?
No, as I've tried to explain earlier, mixing constant and variable width types is wrong in principal and worked on 64-bit arches just by chance.
Comment 15 Nuno Teixeira freebsd_committer freebsd_triage 2022-10-31 12:49:50 UTC
(In reply to Alexey Dokuchaev from comment #14)

I will inform maintainers that use this crate about fix.

Thanks very much!
Comment 16 Alexey Dokuchaev freebsd_committer freebsd_triage 2022-10-31 12:51:32 UTC
(In reply to Nuno Teixeira from comment #15)
> I will inform maintainers that use this crate about fix.
That would be very nice of you, please do.

> Thanks very much!
No problem, always glad to help.
Comment 17 Nuno Teixeira freebsd_committer freebsd_triage 2022-10-31 17:54:00 UTC
devel/git-delta         # 267290
devel/gitui             # 267469
devel/libdatadog        # 267470
devel/wrangler          # 267471
security/solana         # BROKEN rust 1.61.0+
sysutils/topgrade       # 267447
textproc/bat            # Not affected, sys-info crate removed from Cargo.lock
www/deno                # 267474 (Broken for other reason)
Comment 18 commit-hook freebsd_committer freebsd_triage 2022-10-31 17:55:17 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=98da5276cd666ad92398ebaf032a1ed62cf62371

commit 98da5276cd666ad92398ebaf032a1ed62cf62371
Author:     Nuno Teixeira <eduardo@FreeBSD.org>
AuthorDate: 2022-10-31 17:46:28 +0000
Commit:     Nuno Teixeira <eduardo@FreeBSD.org>
CommitDate: 2022-10-31 17:54:16 +0000

    devel/git-delta: Fix i386 build

     - sys-info-0.9.1 crate fix
       https://github.com/FillZpp/sys-info-rs/issues/80

    PR:             267290

 devel/git-delta/Makefile | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
Comment 19 Nuno Teixeira freebsd_committer freebsd_triage 2022-10-31 18:02:26 UTC
Committed, thanks!