Skip to content

Conversation

@jpellegrini
Copy link
Contributor

@jpellegrini jpellegrini commented Sep 21, 2024

Maintainer: @jpellegrini
Compile tested: x86, master, GCC 13
Run tested: MIPS, Netgear WNDR3800, OpenWRT 23.05.4, used the Chicken compiler and interpreter in the target. The binaries csc, chicken-install, chicken-status have been verified to work. Dynamically loading libraries, both from the interpreter and from compiled programs, works.
Patches: A patch is included in order to get the compiler (csc) to work properly on the target device (comment in the OpenWRT package Makefile).

About the patch:

Chicken includes an interpreter and also a compiler, "csc" which calls gcc.

The Chicken build system will dynamically build the file chicken-config.h, and for OpenWRT this is done inside the buildroot. But then, the values in that file are hardcoded into the csc binary, and they don't work well in the target device:

  1. -ldl is passed to ld
  2. -fmacro-prefix-map=... is passed to gcc, with the original path from the buildroot.

These two will not work on OpenWRT (and are not needed anyway), so the patch included actually modifies the build system to remove those two flags. Then csc works on the target device!

@jpellegrini
Copy link
Contributor Author

The test build failed claiming I included a binary patch - but the patch I included actually only changes the build system. What should I do now?

@jpellegrini
Copy link
Contributor Author

jpellegrini commented Sep 21, 2024

the patch I included actually only changes the build system

Which is not a conventional one (it's not a configure-make-make install system). The file patched is not Makefile, Makefile.am or configure.ac... But it's part of the build system (a header, chicken-config.h is written during the compilation process, but it will get confused in the build system and make the compiler try to use, on the target, flags that only make sense in the host -- so my patch changes the script to remove those)

@jpellegrini jpellegrini force-pushed the master branch 2 times, most recently from f7df437 to ec8cc69 Compare September 21, 2024 14:08
@@ -0,0 +1,15 @@
diff -Nur chicken-5.4.0/defaults.make chicken-5.4.0-new/defaults.make
Copy link
Member

Choose a reason for hiding this comment

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

1st) Have you check how other patches in this repository looks like?
That would provide you answers that you are looking for.
2nd) Why do we need to carry this patch in downstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok.

  1. Didn't think the first line would make a difference; I've removed it.
  2. I don't understand the question. Do you mean I should include an explanation of why it's needed, in the PR? I will do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't think the first line would make a difference; I've removed it.

Indeed, it doesn't make a difference when compiling the package... It only shows up when the test bot looks at it (?).

I will do that.

Done!

Copy link
Member

Choose a reason for hiding this comment

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

I havent said anything about the first line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I havent said anything about the first line.

I don't get it. I have run, locally,

make package/chicken-scheme/clean
make package/chicken-scheme/refresh

and it all works fine (no errors, returns status zero). But when the PR goes through the tests, it complains about dirty patches.

It seems that here (my notebook), in the second time it goes through the patch, all is fine:

Applying patch 00.build
patching file defaults.make

Now at patch 00.build
touch "/home/jeronimo/pkg/openwrt/build_dir/target-mips_24kc_musl/chicken-5.4.0/.quilt_checked"
Applying patch 00.build
patching file defaults.make

Now at patch 00.build
Patch 00.build is unchanged
rm -rf /home/jeronimo/pkg/openwrt/package/chicken-scheme/patches 2>/dev/null >/dev/null

but during the automated tests,

  Applying patch 00.build
  patching file defaults.make
  
  Now at patch 00.build
  touch "/builder/build_dir/target-aarch64_generic_musl/chicken-5.4.0/.quilt_checked"
  Applying patch 00.build
  patching file defaults.make
  
  Now at patch 00.build
  Refreshed patch 00.build
  rm -rf /feed/lang/chicken-scheme/patches 2>/dev/null >/dev/null

Why on my notebook it says "Patch 00.build is unchanged" but on github it says "Refreshed patch 00.build"?

Copy link
Member

Choose a reason for hiding this comment

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

This patch is quite obscure. Can it be avoided by passing the right values to make? I read the rest of the comments, but it's not 100% clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello!

This patch is quite obscure. Can it be avoided by passing the right values to make? I read the rest of the comments, but it's not 100% clear.

It can't be done via makefile... The original system assumes a lot of things about the target environment, and I tried hard to find a way to not patch it.

Chicken is a Scheme interpreter and compiler. csi is the interpreter, and csc is the compiler, which translates Scheme into C and calls gcc.

What the patch does is to remove -fmacro-prefix-map and -ldl in the strings that are passed to gcc when the Scheme compiler runs. Without that, the compiler will not run on the router.

I would like to still be able to use scheme in my router, and let others have that resource too :)
So there are these possibilities, I believe (correct me if I'm wrong):

  • keep the patch (including the above explanation in the commit message, and perhaps in the Makefile?)
  • remove the compiler and keep the interpreter only (but the compiler is used when installing new modules, which are downloaded in source form)
  • (would take some time) I'd be willing to package a different Scheme implementation, but several of them do not boostrap from scratch, and require a previous version of themselves installed -- so it complicates things a bit, but it's possible

What is preferable?

(I have been working on porting languages to OpenWRT as a personal project: https://gitlab.com/jpellegrini/openwrt-packages -- Algol, BASIC, Forth, Scheme... Even a text-based windowing system)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a patch header with a short explanation for others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe add a patch header with a short explanation for others.

Okay! Also included the explanation in the commit message.

@GeorgeSapkin
Copy link
Member

Looks like the patch could use a refresh.

@jpellegrini
Copy link
Contributor Author

Looks like the patch could use a refresh.

Yes. I'll try to get this fixed in the next days.

@GeorgeSapkin
Copy link
Member

Any reason why these are not in DEPENDS? EXTRA_DEPENDS are for specifying additional constrains, like a specific version, which these don't have.

EXTRA_DEPENDS:= gcc, coreutils-install

@jpellegrini
Copy link
Contributor Author

Any reason why these are not in DEPENDS?

I'm not sure what that means -- I was actually going to investigate what the problem is.

@jpellegrini
Copy link
Contributor Author

I'm not sure what that means -- I was actually going to investigate what the problem is.

I can't see why the build system is complaining. The only patch I included is lang/chicken-scheme/patches/00.build, which is:

--- a/defaults.make
+++ b/defaults.make
@@ -506,6 +506,11 @@
 endif
 	$(call echo, >>, $@,#endif)
 	$(call echo, >>, $@,/* END OF FILE */)
+	# For OpenWRT:
+	sed -e's/\-fmacro-prefix-map[^ ]*//' $@ > [email protected]
+	sed -e's/\-ldl//' [email protected] > [email protected]
+	cp [email protected] $@
+	rm [email protected] [email protected]
 
 chicken-install.rc:
 	$(call echo, >, $@,/* GENERATED */)

The patch includes some changes to a file in the sources, but it cleans after itself (rm [email protected] [email protected]). What else could be going on?

@GeorgeSapkin
Copy link
Member

Did you try make package/chicken-scheme/refresh and pushing those changes? The CI is telling you that the patch can be refreshed, i.e. it's not properly formatted or doesn't apply cleanly.

@jpellegrini
Copy link
Contributor Author

Did you try make package/chicken-scheme/refresh

Oooooh that's what was missing. Sure It changed one tiny difference in the patch, and it seems to work now (one of the previously failing tests already passed). Thank you!

@GeorgeSapkin
Copy link
Member

Any reason why these are not in DEPENDS?

I'm not sure what that means -- I was actually going to investigate what the problem is.

If a package depends on something it should use DEPENDS and not EXTRA_DEPENDS, because you're not specifying additional i.e. extra constraints. Please change it.

@jpellegrini
Copy link
Contributor Author

If a package depends on something it should use DEPENDS and not EXTRA_DEPENDS, because you're not specifying additional i.e. extra constraints. Please change it.

Ah ok. I just pushed it.

@GeorgeSapkin
Copy link
Member

Also it looks like there's no CI test script - test.sh. Please add that as a separate commit, ideally before the version update one.

It should be something like:

#!/bin/sh

csc --version | grep -F "$2"

Assuming csc is the name of the binary and it has a --version flag.

@jpellegrini
Copy link
Contributor Author

OK, will do later

@jpellegrini
Copy link
Contributor Author

Also it looks like there's no CI test script - test.sh. Please add that as a separate commit, ideally before the version update one.

I'm sorry, I have reviewed https://openwrt.org/docs/guide-developer/start but couldn't find any reference to that. Where do I include such test file? Also, would it be a dummy test, or can I include a call to the package's test suite?

@GeorgeSapkin
Copy link
Member

There is an example in my previous comment. It's just a script called test.sh in the root of the package, next to the Makefile. Check some other packages for inspiration.

@jpellegrini
Copy link
Contributor Author

Added a simple CI test script. Will do better in a next version

@jpellegrini jpellegrini force-pushed the master branch 2 times, most recently from b4488c3 to 3e9feab Compare November 20, 2025 00:18
It just checks the version for now...

Signed-off-by: Jeronimo Pellegrini <[email protected]>
* New version.
* A patch is included in order to get the compiler (csc) to
  work properly on the target device (comment in the OpenWRT
  package Makefile). csc, chicken-install, chicken-status have
  been verified to work.
  What the patch does is to remove -fmacro-prefix-map and -ldl
  in the strings that are passed to gcc when the Scheme
  compiler runs. Without that, the compiler will not run on
  the router.

Signed-off-by: Jeronimo Pellegrini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants