[-- Attachment #1: Type: text/plain, Size: 1451 bytes --] As mentioned previous e-mails, either: • the docstring forgets to mention that it is the caller’s responsibility it is to check that ‘str’ is a valid method name (and you need to check/adjust the callers to do the check, of which I have seen n) • or 'parse-http-method’ is missing a check that (string-length str)>0 and the characters of str belong to tchar (defined in the RFC). Please do not ignore reviews when making PRs – disagreeing with some review(s) and then mentioning in the commit message/cover letter/comments/... as appropriate why you deviate from them is one thing, just acting as if they didn’t happen is another. In the previous discussion I have heard an argument for not doing the check, which basically amounts to flexibility for sake of flexibility in spite of potential security problems and in spite of the third option (i.e. with both security and flexibility) of not allowing it by default and instead adding an argument somewhere to add a custom checker/list of extra allowed methods/... that defaults to what RFC specifies, but I haven’t heard an argument for not properly documenting that what parse-http-method parses aren’t HTTP methods so the callers may need to do extra validation. Also, the commit message needs to be changed to changelog format. Also worth checking if the Texinfo documentation mentions parse-http-method, and if so, adjust it. Best regards, Maxime Devos. [-- Attachment #2: Type: text/html, Size: 6016 bytes --]
[-- Attachment #1: Type: text/plain, Size: 772 bytes --] I'm not sure how pull-requests are handled in this project, but Maxime suggested I send the PR to this mailing list. The following changes since commit 48548df91e9eb5d4a46391da0ad0a8cdd3387857: Fix effects analysis: field writes clobber object reads (2024-03-20 11:50:53 +0100) are available in the Git repository at: https://git.sr.ht/~rjraymond/guile for you to fetch changes up to b19ff6b1618de5b5abe13188ce0e359c07215d6b: Allow extended HTTP methods as mentioned in RFC 9110 16.1 (2024-03-28 00:34:21 -0400) ---------------------------------------------------------------- Ryan Raymond (1): Allow extended HTTP methods as mentioned in RFC 9110 16.1 module/web/http.scm | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) Ryan [-- Attachment #2: Type: text/html, Size: 990 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1589 bytes --] Oh I'm so embarrassed. I thought git pull-request sent an email to the git server the same way one might push changes. I didn't realize I would need to copy that pull request message and send it as an email. Thanks Maxime. I feel like you've really done a lot to help me out and I appreciate it. On Wed, Mar 27, 2024, 10:15 AM Maxime Devos <maximedevos@telenet.be> wrote: > >I am trying to contribute to Guile but I cannot figure out how I am > supposed to do it. I submitted a pull request, but I cannot see the status > of that request on cgit. Do we even use pull requests or do we do pushes? > So I need to get put on the member list by filing a request for inclusion? > > > > Where can people find this pull request? Like, is this a ‘git > request-pull’-style PR send by e-mail, a ‘git request-pull’-style PR that > was formatted by ‘git request-pull’ but you forgot to send, a PR on an > unofficial GitHub mirror that almost nobody looks at, some Savannah feature > unknown to me, or ... > > > > > do we do pushes? > > > > If with this you mean that people with contributions directly push it to > the main branch: no. > > > > > So I need to get put on the member list by filing a request for > inclusion? > > > > No. > > > > I have done contributions in the past without having an account on > Savannah and without any kind of administration – I just send patches to > guile-devel@gnu.org with a cover message. (Or without cover message if > it’s just a single patch.) > > > > Best regards, > > Maxime Devos. > [-- Attachment #2: Type: text/html, Size: 2508 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1099 bytes --] >I am trying to contribute to Guile but I cannot figure out how I am supposed to do it. I submitted a pull request, but I cannot see the status of that request on cgit. Do we even use pull requests or do we do pushes? So I need to get put on the member list by filing a request for inclusion? Where can people find this pull request? Like, is this a ‘git request-pull’-style PR send by e-mail, a ‘git request-pull’-style PR that was formatted by ‘git request-pull’ but you forgot to send, a PR on an unofficial GitHub mirror that almost nobody looks at, some Savannah feature unknown to me, or ... > do we do pushes? If with this you mean that people with contributions directly push it to the main branch: no. > So I need to get put on the member list by filing a request for inclusion? No. I have done contributions in the past without having an account on Savannah and without any kind of administration – I just send patches to guile-devel@gnu.org with a cover message. (Or without cover message if it’s just a single patch.) Best regards, Maxime Devos. [-- Attachment #2: Type: text/html, Size: 4769 bytes --]
[-- Attachment #1: Type: text/plain, Size: 528 bytes --] Hello, I am trying to contribute to Guile but I cannot figure out how I am supposed to do it. I submitted a pull request, but I cannot see the status of that request on cgit. Do we even use pull requests or do we do pushes? So I need to get put on the member list by filing a request for inclusion? There are limitations to the language that have forced me to use a modified version to complete my projects and I would be thrilled to share those enhancements with the community. Does anyone have any advice in this regard? Ryan [-- Attachment #2: Type: text/html, Size: 610 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1156 bytes --] Hello Arsen, > ... > This patch is in a similar vein to my earlier patch to Guile itself > posted at > https://lists.gnu.org/archive/html/guile-devel/2023-03/msg00040.html > These came up while I was working on packaging Guile in Gentoo. > ... I pushed the proposed changes to the devel branch, will be part of the next release, thanks. > Also, on a similar note, guile-lib installs to /usr/share/guile/site/ > rather than /usr/share/guile/site/$EV (i.e. %global-site-dir instead > of %site-dir) like other packages do. Why is that the case? Because, as you spotted yourself, guile-lib src modules are guile version agnostic ... But i'll think about it, as it appears i am a bit lonely, to think one shouldn't (ever) duplicate guile version agnostic code (on the same machine), as doing so could lead to other (potentially serious) problem(s) ... Meanwhile, as all distro do, feel free to patch this downstream, to install the scr modules in $prefix/share/guile/site/$EV - but the ideal would be to have 'the machinery' to properly handle one version of the (identical) src modules, multiple .go versions ... David [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1625 bytes --] Your flag #:blocking? #true would do a per-connection thread, which while implementing lack of blocking, is a stronger property than non-blocking. Also, (kernel-level) threads have some overhead, which is sub-optimal; Instead of a flag, I propose defining a new ‘server-impl’ Web Server (Guile Reference Manual) (gnu.org) This impl could then spawn threads (or, more likely, do some thread pool things, presumably with some mitigations against ‘read one byte, wait N minutes’ DDOS attacks). Also, single-threaded does not imply blocking (assuming the implementation uses ‘select’ / ‘epoll’ / etc. -- I don’t know if Guile does that yet, might be worth checking). If the respond code writes to ports, some ‘(parameterize ((current-read-waiter ...) (current-write-waiter ...)) ...)’ + delimited continuations + O_NONBLOCK will be needed if the respond code writes to port. This, by the way, is already implemented by Guile-Fibers. I recommend using Guile-Fibers HTTP implementation or something based upon this (it has (relatively?) lightweight M:N user-level threads). I don’t recommend the default Fibers implementation, because it does ‘run-fibers’ and create threads on its own, which is really not its responsibility and as such makes it inconvenient to use when Fibers is used for other things as well. Instead, I am currently using fiberized.scm « server « web - gnunet-scheme.git - GNUnet client implementation in (Guile) Scheme (I don’t recall which changes I made). I’ve only used it for demo purposes so far, though. Best regards, Maxime Devos. [-- Attachment #2: Type: text/html, Size: 4271 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1526 bytes --] Hi Ryan! For the single-threaded non-blocking server in Guile, you have to make sure at least 3 steps: 1. set socket to non-blocking 2. enable suspended ports to prepare delimited-continuation powered coroutine 3. designed your own scheduler Even in the single thread, you can handle requests concurrently because of the coroutine based on delimited-continuation. Of course you may enhance it with thread pool. Best regards. On Mon, Mar 25, 2024 at 4:04 AM Ryan Raymond <rjraymond@oakland.edu> wrote: > Hello, all. > I was able to build a non-blocking web-server using network sockets. > However, the existing guile web/server.scm implementation is > single-threaded and therefore blocking, which is sub-optimal for some > use-cases. > > I suggest we slightly modify the server logic to have an optional > #:blocking? [bool] parameter which would enable behavior in line with the > following excerpt from my own code. Obviously some changes would be made to > methodology and code style, but you get the picture. > > (define (listen sock) > (let* ( > (client-connection (accept sock)) > (client-details (cdr client-connection)) > (client (car client-connection))) > (begin-thread > (sigaction SIGPIPE SIG_IGN) > (handle client) > (close client)) > )) > > This shouldn't cause any backwards-compatibility issues since it's > optional, but the specifics of web/server.scm might cause problems. > Let me know what you think! > Ryan > [-- Attachment #2: Type: text/html, Size: 2143 bytes --]
[-- Attachment #1: Type: text/plain, Size: 563 bytes --] On Sun, Mar 24, 2024 at 03:41:32PM -0400, Ryan Raymond wrote: > Hello, all. > I was able to build a non-blocking web-server using network sockets. > However, the existing guile web/server.scm implementation is > single-threaded and therefore blocking [...] How does "single-threaded" imply "blocking"? I mean: multithreading does have its uses, no question (especially if you want to leverage more than one CPU core). As one example, Nginx, arguably the fastest of the web servers out there isn't based on threads for concurrency. Cheers -- t [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #1: Type: text/plain, Size: 661 bytes --] Hello Vagrant, Sorry it took so long to answer, I forgot, and only recently remembered, as Maxime started to work on the logger, that someone did send a patch to fix a typo ... > Originally sent to guile-user, as the README suggested bugs should go > there, though I see guile-devel mentioned on > https://www.nongnu.org/guile-lib/dev/ Both the typo and the pronoun (as reported in a private email by Keith Wright) have been fixed, thanks to both of you. > Maybe the README should be updated? Indeed, as well as their occurrence in the INSTALL and NEWS files, all fixed (devel branch now, be part of the next release), thanks. David [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #1: Type: text/plain, Size: 919 bytes --] Hello, all. I was able to build a non-blocking web-server using network sockets. However, the existing guile web/server.scm implementation is single-threaded and therefore blocking, which is sub-optimal for some use-cases. I suggest we slightly modify the server logic to have an optional #:blocking? [bool] parameter which would enable behavior in line with the following excerpt from my own code. Obviously some changes would be made to methodology and code style, but you get the picture. (define (listen sock) (let* ( (client-connection (accept sock)) (client-details (cdr client-connection)) (client (car client-connection))) (begin-thread (sigaction SIGPIPE SIG_IGN) (handle client) (close client)) )) This shouldn't cause any backwards-compatibility issues since it's optional, but the specifics of web/server.scm might cause problems. Let me know what you think! Ryan [-- Attachment #2: Type: text/html, Size: 1099 bytes --]
[-- Attachment #1: Type: text/plain, Size: 4054 bytes --] >The character-set you're referring to, is it US-ASCII? I am not particularly familiar with how Guile handles characters. If string-filter is not sufficient, can you suggest another method? > >For example, perhaps we need to go to where "str" is read and set the port encoding to US-ASCII. Right now it's Iso Latin which is a superset of US-ASCII, and therefore improper. I meant “character set” in the sense as used in Guile, not character encoding. Very literally, it means a “set of characters”, where ‘set’ is used in the mathematical sense. ‘Character’ means any character in Unicode (not counting those special pairs used for UTF-16, they aren’t characters). Given you mentioned char-set:graphic, I thought you already knew. So, the answer is, no, it’s not ASCII (the character set), it’s a subset of US-ASCII defined in the HTTP spec. IIRC, I referred to: ➢ https://www.rfc-editor.org/rfc/rfc9110.html#name-tokens (in particular see ‘tchar’) which I think is pretty clearly not all of ASCII but rather a subset. Explicitly, the character set I’m referring to is the ‘tchar’ mentioned in the RFC. On string-filter: I suppose you could use that, (string=? (string-filter the-char-set ...) original-string), to check things, but it seems more efficient and simpler to use the predicate string-every instead. That said, it might be worth looking at how the caller(s) of the method parsing procedure uses the method parsing procedure. It might be the case that they use something to (string-index s everything-except-tchar begin end) to locate the end of the method name. In that case, the argument passes to the method parsing procedure is correct by construction (assuming length>0), so then that procedure doesn’t need to do any checks and can leave (with a docstring) that responsibility to the caller. >For example, perhaps we need to go to where "str" is read and set the port encoding to US-ASCII. Right now it's Iso Latin which is a superset of US-ASCII, and therefore improper. Eh, while HTTP might look like text, it’s more like a mix of text and octets/bytes: Field values are usually constrained to the range of US-ASCII characters [USASCII]. __Fields needing a greater range of characters can use an encoding__, such as the one defined in [RFC8187]. Historically, HTTP allowed field content with text in the __ISO-8859-1__ charset [ISO-8859-1], supporting other charsets only through use of [RFC2047] encoding. Specifications for newly defined fields SHOULD limit their values to visible US-ASCII octets (VCHAR), SP, and HTAB. __A recipient SHOULD treat other allowed octets in field content (i.e., obs-text) as opaque data__. (emphasis added) I interpret this as “HTTP prefers only US-ASCII(see SHOULD), but it’s not strictly required (depending on the field), and sometimes it doesn’t even have any meaning as characters and instead is only raw bytes(*)”. Also see the bit about ISO-8559-1, it appears that in at least some case, the ISO-8559-1 encoding should be recognised. (I might be misinterpreting this though, perhaps it is referring to %-encoding.) Also, using ISO Latin 1 (or another ASCII (the character encoding)-compatible 8-bit encoding) is convenient for handling octets and US-ASCII characters together. Maybe separating the US-ASCII from the extra octets might make the code more proper in some aesthetical sense, but I don’t think it would make things more proper in a RFC-compliant sense (though neither would it make things worse, I suppose). (There might be bugs w.r.t. character encoding in the Guile implementation, but I don’t think this is one of them.) >That being said, the best form for this function is: >(string->symbol (substring str start end) ) >With additional logic added to other functions? I am not familiar enough with the Guile implementation to tell if the extra logic is best done in this function or in its caller. It just needs to be done _somewhere_. Best regards, Maxime Devos [-- Attachment #2: Type: text/html, Size: 9098 bytes --]
[-- Attachment #1: Type: text/plain, Size: 835 bytes --] (else (bad-request "Invalid method: ~a" (substring str start end))))) Another problem with the old implementation – AFAICT, this does a “400 Bad Request”. However, at least when the syntax/grammar/... is correct, it should be a 501 instead. >An origin server that receives a request method that is unrecognized or not implemented SHOULD respond with the 501 (Not Implemented) status code. An origin server that receives a request method that is recognized and implemented, but not allowed for the target resource, SHOULD respond with the 405 (Method Not Allowed) status code. Given the proposal to (in this procedure) allow all methods (at least all grammatical methods) in a new implementation, it seems this won’t be this method’s responsibility anymore, though. Best regards, Maxime Devos [-- Attachment #2: Type: text/html, Size: 3207 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1279 bytes --] On Wed, 2024-03-20 at 16:40 -0400, Thompson, David wrote: > On Wed, Mar 20, 2024 at 4:29 PM Jonas Hahnfeld <hahnjo@hahnjo.de> wrote: > > So I can confirm that JIT indeed doesn't work right now on 64-bit > > MinGW, but it's relatively easy to fix (first patch). In essence > > lightening was getting the calling convention wrong. > > Wow! Have you seen the JIT do its thing (via GUILE_JIT_LOG) or just > verified that compilation succeeds when JIT is enabled? Either way, a > big step forward. The patch is very simple, too. I had only verified that the produced LilyPond executable still worked, but I can now confirm that setting GUILE_JIT_LOG shows that something is happening. I don't have performance data on this yet, I asked the community to test the version on larger inputs. > > Compilation just works --with-threads, as long as bdwgc was built with > > --enable-threads=posix to override the automatic detection of win32 > > threading. I haven't tested if it actually works, but there might be a > > good chance. > > This is also encouraging! Anyone out there want to run a test using > call-with-new-thread? So for the fun, I tried compiling --with-threads again and (call-with- new-thread) seems to return new threads. Cheers Jonas [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2165 bytes --] While I agree it has advantages, it is also buggy. From https://www.rfc-editor.org/rfc/rfc9110.html#name-methods: * “the method token is case-sensitive”. So, you need to remove ‘string-upcase’. * Additional methods, outside the scope of this specification, have been specified for use in HTTP. All such methods ought to be registered within the "Hypertext Transfer Protocol (HTTP) Method Registry", as described in Section 16.1. (Just a reminder, not a bug with the proposed new parse-http-method.) If it’s something application-specific, I propose naming the method something like “APP-METHOD” (where APP = name of APP, METHOD = WAIT / SUBSCRIBE / ...), to avoid potential conflicts. * “method = token” “token” is defined in https://www.rfc-editor.org/rfc/rfc9110.html#name-tokens. So, there are some limitations on what strings are valid method names that you need to verify. For example, suppose that someone uses “GET\0sneaky” as header, and the Guile code places some limits (in terms of 403 forbidden) of what can be read. “GET\0sneaky” passes through because it’s not “GET”, “HEAD” etc.. Let’s say the actual response is constructed by C code, so Guile passes the method to the C-code as a 0-terminated string (as is conventional) and the C-code then interprets “GET\0sneaky” as “GET\0” and sends the response while it shouldn’t. (I don’t know if this particular situation can happen because perhaps parse-http-method is always used in a context where the string is for some reason already a token, but that’s not something to rely upon) * A recipient SHOULD parse a received protocol element defensively, with only marginal expectations that the element will conform to its ABNF grammar and fit within a reasonable buffer size. (quoting this for the “conform to grammar” part, not the “buffer size part”) (With regards to the first point) this paragraph SHOULD be ignored, this way leads to continued violations of the grammar and potential security problems (see example in previous point). Postel’s law is a mistake. Best regards, Maxime Devos [-- Attachment #2: Type: text/html, Size: 8765 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1435 bytes --] Arne, you're my hero. Meta/guile does include my changes *and* has a quicker turnaround for testing changes than building every time. Thank you! As for the dependencies, make clean is what I used and that didn't reflect the changes I made. I will try more things before I submit a pull request but at least I can continue my experiments. Thanks Ryan On Thu, Mar 21, 2024, 12:47 PM Dr. Arne Babenhauserheide <arne_bab@web.de> wrote: > > Ryan Raymond <rjraymond@oakland.edu> writes: > > > For example, I modified "parse-http-method" and completely removed all > error throwing capabilities but I am still getting an error thrown from > > within that function. > > (bad-request "Invalid method: ~a" (substring str start end)) > > > > I am assuming that the modules are not being rebuilt. My workflow is as > follows: > > 1. Modify the source code > > 2. ./configure > > 3. make > > 4. make install > > > > Can anyone point out a mistake I might be making? > > A shot into the blue would be that some dependencies aren’t marked > correctly in the Makefile so some file is not being rebuilt and includes > the old version of the function via a macro. > > Does it also happen when you add a `make clean` step? > > Does it happen when you directly run meta/guile instead of installing? > > Best wishes, > Arne > -- > Unpolitisch sein > heißt politisch sein, > ohne es zu merken. > draketo.de > [-- Attachment #2: Type: text/html, Size: 2058 bytes --]
[-- Attachment #1: Type: text/plain, Size: 929 bytes --] Ryan Raymond <rjraymond@oakland.edu> writes: > For example, I modified "parse-http-method" and completely removed all error throwing capabilities but I am still getting an error thrown from > within that function. > (bad-request "Invalid method: ~a" (substring str start end)) > > I am assuming that the modules are not being rebuilt. My workflow is as follows: > 1. Modify the source code > 2. ./configure > 3. make > 4. make install > > Can anyone point out a mistake I might be making? A shot into the blue would be that some dependencies aren’t marked correctly in the Makefile so some file is not being rebuilt and includes the old version of the function via a macro. Does it also happen when you add a `make clean` step? Does it happen when you directly run meta/guile instead of installing? Best wishes, Arne -- Unpolitisch sein heißt politisch sein, ohne es zu merken. draketo.de [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 1125 bytes --]
[-- Attachment #1: Type: text/plain, Size: 644 bytes --] Hello, all. Earlier I mentioned a patch I wanted to apply to the guile source code. I have implemented my change but the guile binary I build with the default build instructions still has the unmodified function. For example, I modified "parse-http-method" and completely removed all error throwing capabilities but I am still getting an error thrown from within that function. (bad-request "Invalid method: ~a" (substring str start end)) I am assuming that the modules are not being rebuilt. My workflow is as follows: 1. Modify the source code 2. ./configure 3. make 4. make install Can anyone point out a mistake I might be making? Ryan [-- Attachment #2: Type: text/html, Size: 844 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1458 bytes --] Hello, all. Please consider the following code snippet from /module/web/http.scm: (define* (parse-http-method str #:optional (start 0) (end (string-length str))) "Parse an HTTP method from STR. The result is an upper-case symbol, like ‘GET’." (cond ((string= str "GET" start end) 'GET) ((string= str "HEAD" start end) 'HEAD) ((string= str "POST" start end) 'POST) ((string= str "PUT" start end) 'PUT) ((string= str "DELETE" start end) 'DELETE) ((string= str "OPTIONS" start end) 'OPTIONS) ((string= str "TRACE" start end) 'TRACE) ((string= str "CONNECT" start end) 'CONNECT) ((string= str "PATCH" start end) 'PATCH) (else (bad-request "Invalid method: ~a" (substring str start end))))) I do not understand why it is necessary to throw an error on non-standard http methods. I am working on an application where I will use (for example) "WAIT" and "SUBSCRIBE". Why not replace the code with, for example: (define* (parse-http-method str #:optional (start 0) (end (string-length str))) "Parse an HTTP method from STR. The result is an upper-case symbol, like ‘GET’." (string->symbol (string-upcase (substring str start end)))) This has many advantages. It is probably faster, allows for lowercase methods (maybe useful?), it has far fewer LOC, and most importantly it allows us to have custom methods, which is extremely useful. Please weigh in with your opinions. Ryan [-- Attachment #2: Type: text/html, Size: 1853 bytes --]
On Wed, Mar 20, 2024 at 4:29 PM Jonas Hahnfeld <hahnjo@hahnjo.de> wrote: > > So I can confirm that JIT indeed doesn't work right now on 64-bit > MinGW, but it's relatively easy to fix (first patch). In essence > lightening was getting the calling convention wrong. Wow! Have you seen the JIT do its thing (via GUILE_JIT_LOG) or just verified that compilation succeeds when JIT is enabled? Either way, a big step forward. The patch is very simple, too. > Compilation just works --with-threads, as long as bdwgc was built with > --enable-threads=posix to override the automatic detection of win32 > threading. I haven't tested if it actually works, but there might be a > good chance. This is also encouraging! Anyone out there want to run a test using call-with-new-thread? > For the one known issue I mentioned above, the fix is also not too > difficult, just being a bit more careful with mixing scm_t_inum and > long (see second patch). The patch makes sense to me! > I'm also explicitly CC'ing Andy and Ludo - we really need a statement > by a maintainer whether this can land. From my point of view, it's a > clear improvement in terms of supported platforms, plus tested by > LilyPond since some time now which is probably one of the bigger > "customers". +1 on hearing from Andy and/or Ludo about this. Jonas' patches are small and should be relatively quick to review. :) If this stuff lands I will have to find a Windows machine to try it out sometime. Thanks Jonas! - Dave
[-- Attachment #1.1: Type: text/plain, Size: 3548 bytes --] On Wed, 2024-02-07 at 21:29 +0100, Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library wrote: > On Wed, 2024-02-07 at 15:23 -0500, Thompson, David wrote: > > On Wed, Feb 7, 2024 at 3:19 PM Jonas Hahnfeld <hahnjo@hahnjo.de> wrote: > > > > > > On Wed, 2024-02-07 at 09:19 -0500, Thompson, David wrote: > > > > On Thu, Jan 4, 2024 at 5:40 AM Jonas Hahnfeld wrote: > > > > > > > > > > On Tue, 2023-11-28 at 22:04 +0100, Jonas Hahnfeld wrote: > > > > > > > > > > > > Ping, any comments on this approach? I built binaries for LilyPond > > > > > > 2.25.10 using these patches applied on top of Guile 3.0.9 and the > > > > > > result seems to work fine on Windows. > > > > > > > > > > Another ping; meanwhile we switched to building the official binaries > > > > > of LilyPond with Guile 3.0 starting from version 2.25.11, but it would > > > > > be really great to get rid of our downstream patches... > > > > > > > > Just chiming in to say this is a very exciting development that I had > > > > missed when the patch set was first sent! > > > > > > > > Does this allow a fully featured Guile build or are some things still > > > > disabled? Does JIT work? > > > > > > It's functional enough to run LilyPond (which uses quite a bit of > > > Guile) and well enough so that there is only one complaint (that I know > > > of so far) about multiplication with negative numbers not working > > > right. If I remember correctly from quickly having a look, that's > > > related to scm_integer_mul_ii using long_magnitude which doesn't quite > > > work on Windows 64-bit. For LilyPond, we disable some features (JIT, > > > threading, networking; you can look at the full build recipe here: > > > https://gitlab.com/lilypond/lilypond/-/blob/master/release/binaries/lib/dependencies.py#L628 > > > ) and I don't know which of these would work or how much it would take > > > to support them. > > > > Ah, bummer. That's a lot of disabled features. JIT and threads are > > must-haves for my use-cases. I guess I'll continue waiting for > > someone to figure out how to build a fully featured Guile on Windows. > > Any takers? ;) > > Isn't the common wisdom that you should learn to crawl and walk before > starting to run? 😉 first we need a booting and working base of Guile > before the rest can follow at some point. For LilyPond's binaries, we > disable networking and threads on all platforms so these may just work. > Regarding the JIT, I think I disabled it right away when I started > trying to understand what the issues were, so it's possible that the > state is not actually that dire... So I can confirm that JIT indeed doesn't work right now on 64-bit MinGW, but it's relatively easy to fix (first patch). In essence lightening was getting the calling convention wrong. Compilation just works --with-threads, as long as bdwgc was built with --enable-threads=posix to override the automatic detection of win32 threading. I haven't tested if it actually works, but there might be a good chance. For the one known issue I mentioned above, the fix is also not too difficult, just being a bit more careful with mixing scm_t_inum and long (see second patch). I'm also explicitly CC'ing Andy and Ludo - we really need a statement by a maintainer whether this can land. From my point of view, it's a clear improvement in terms of supported platforms, plus tested by LilyPond since some time now which is probably one of the bigger "customers". Jonas [-- Attachment #1.2: 0001-Fix-lightening-x86_64-Windows-calling-convention.patch --] [-- Type: text/x-patch, Size: 2711 bytes --] From e701b1583052102a5e1758394e4c3a1fb7565d27 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld <hahnjo@hahnjo.de> Date: Wed, 20 Mar 2024 20:26:36 +0100 Subject: [PATCH 1/2] Fix lightening x86_64 Windows calling convention * libguile/lightening/lightening/x86.c: * libguile/lightening/lightening/x86.h: Check _WIN64 macro as set for mingw64. --- libguile/lightening/lightening/x86.c | 10 +++++----- libguile/lightening/lightening/x86.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/libguile/lightening/lightening/x86.c b/libguile/lightening/lightening/x86.c index f8ac4b0b8..6963d90fd 100644 --- a/libguile/lightening/lightening/x86.c +++ b/libguile/lightening/lightening/x86.c @@ -237,7 +237,7 @@ jit_init(jit_state_t *_jit) static const jit_gpr_t abi_gpr_args[] = { #if __X32 /* No GPRs in args. */ -#elif __CYGWIN__ +#elif defined(__CYGWIN__) || defined(_WIN64) _RCX, _RDX, _R8, _R9 #else _RDI, _RSI, _RDX, _RCX, _R8, _R9 @@ -247,7 +247,7 @@ static const jit_gpr_t abi_gpr_args[] = { static const jit_fpr_t abi_fpr_args[] = { #if __X32 /* No FPRs in args. */ -#elif __CYGWIN__ +#elif defined(__CYGWIN__) || defined(_WIN64) _XMM0, _XMM1, _XMM2, _XMM3 #else _XMM0, _XMM1, _XMM2, _XMM3, _XMM4, _XMM5, _XMM6, _XMM7 @@ -317,7 +317,7 @@ reset_abi_arg_iterator(struct abi_arg_iterator *iter, size_t argc, memset(iter, 0, sizeof *iter); iter->argc = argc; iter->args = args; -#if __CYGWIN__ && __X64 +#if (defined(__CYGWIN__) || defined(_WIN64)) && __X64 // Reserve slots on the stack for 4 register parameters (8 bytes each). iter->stack_size = 32; #endif @@ -330,12 +330,12 @@ next_abi_arg(struct abi_arg_iterator *iter, jit_operand_t *arg) enum jit_operand_abi abi = iter->args[iter->arg_idx].abi; if (is_gpr_arg(abi) && iter->gpr_idx < abi_gpr_arg_count) { *arg = jit_operand_gpr (abi, abi_gpr_args[iter->gpr_idx++]); -#ifdef __CYGWIN__ +#if defined(__CYGWIN__) || defined(_WIN64) iter->fpr_idx++; #endif } else if (is_fpr_arg(abi) && iter->fpr_idx < abi_fpr_arg_count) { *arg = jit_operand_fpr (abi, abi_fpr_args[iter->fpr_idx++]); -#ifdef __CYGWIN__ +#if defined(__CYGWIN__) || defined(_WIN64) iter->gpr_idx++; #endif } else { diff --git a/libguile/lightening/lightening/x86.h b/libguile/lightening/lightening/x86.h index 983ebdb8f..7da8c5977 100644 --- a/libguile/lightening/lightening/x86.h +++ b/libguile/lightening/lightening/x86.h @@ -92,7 +92,7 @@ # define JIT_F6 _XMM6 # define JIT_FTMP _XMM7 # define JIT_PLATFORM_CALLEE_SAVE_GPRS JIT_TMP0 -#elif __CYGWIN__ +#elif defined(__CYGWIN__) || defined(_WIN64) # define JIT_R0 _RAX # define JIT_R1 _RCX # define JIT_R2 _RDX -- 2.44.0 [-- Attachment #1.3: 0002-Use-inum_magnitude-for-inums.patch --] [-- Type: text/x-patch, Size: 1975 bytes --] From cf380c1a734acd0d22041c7c875fbffc6b535d2a Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld <hahnjo@hahnjo.de> Date: Wed, 20 Mar 2024 20:57:02 +0100 Subject: [PATCH 2/2] Use inum_magnitude for inums * libguile/integers.c: Call inum_magnitude instead of long_magnitude for scm_t_inum arguments. --- libguile/integers.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libguile/integers.c b/libguile/integers.c index 23bd2c0d5..380ff193c 100644 --- a/libguile/integers.c +++ b/libguile/integers.c @@ -251,7 +251,7 @@ inum_to_bignum (scm_t_inum i) if (i > 0) return ulong_to_bignum (i); - return i == 0 ? make_bignum_0 () : make_bignum_1 (1, long_magnitude (i)); + return i == 0 ? make_bignum_0 () : make_bignum_1 (1, inum_magnitude (i)); #else return make_bignum_from_int64 (i); #endif @@ -3061,15 +3061,15 @@ scm_integer_mul_ii (scm_t_inum x, scm_t_inum y) return SCM_I_MAKINUM (k); #endif - mp_limb_t xd[1] = { long_magnitude (x) }; + mp_limb_t xd[1] = { inum_magnitude (x) }; mp_limb_t lo; int negative = (x < 0) != (y < 0); - mp_limb_t hi = mpn_mul_1 (&lo, xd, 1, long_magnitude (y)); + mp_limb_t hi = mpn_mul_1 (&lo, xd, 1, inum_magnitude (y)); if (!hi) { if (negative) { - if (lo <= long_magnitude (SCM_MOST_NEGATIVE_FIXNUM)) + if (lo <= inum_magnitude (SCM_MOST_NEGATIVE_FIXNUM)) return SCM_I_MAKINUM (negative_long (lo)); } else if (lo <= SCM_MOST_POSITIVE_FIXNUM) @@ -3100,7 +3100,7 @@ scm_integer_mul_zi (struct scm_bignum *x, scm_t_inum y) struct scm_bignum *result = allocate_bignum (xn + 1); mp_limb_t *rd = bignum_limbs (result); const mp_limb_t *xd = bignum_limbs (x); - mp_limb_t yd = long_magnitude (y); + mp_limb_t yd = inum_magnitude (y); int negate = bignum_is_negative (x) != (y < 0); mp_limb_t hi = mpn_mul_1 (rd, xd, xn, yd); if (hi) -- 2.44.0 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #1: Type: text/plain, Size: 378 bytes --] On Thu, 2024-02-22 at 22:14 +0100, Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library wrote: > Dear Guile maintainers, > > please find attached two patches for changes that we have been carrying > downstream in LilyPond so far: one fix to make a sed invocation fully > portable, and a fix for cross-compilation in out-of-tree-builds. ping... [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #1: Type: text/plain, Size: 770 bytes --] On Thu, 2024-03-14 at 17:11 +0600, Nikita Domnitskii wrote: > --- > module/ice-9/eval-string.scm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/module/ice-9/eval-string.scm b/module/ice-9/eval-string.scm > index ea0f17777..9cac03632 100644 > --- a/module/ice-9/eval-string.scm > +++ b/module/ice-9/eval-string.scm > @@ -81,7 +81,7 @@ > (if line > (set-port-line! port line)) > (if column > - (set-port-column! port line)) > + (set-port-column! port column)) > > (if (or compile? (not (language-evaluator lang))) > ((load-thunk-from-memory > > LGTM (wrong since 2011, it seems), but needs somebody with commit access to push. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --]
Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> writes: > Would something like that be OK instead?: Not sure I understand the question. Were you asking if that'd be OK elsewhere in the function too? > However here (char=? ch #\:) can fail if ch is an eof-object. Apologies for the delay, and good point. I should have used eqv? rather than char=?, i.e. (let ((ch (read-char port))) (when (eqv? ch #\:) (set! ch (read-char port)) (if (eof-object? ch) (time-error 'string->date 'bad-date-template-string (list "Invalid time zone number" ch))) (set! ...)) -- Rob Browning rlb @defaultvalue.org and @debian.org GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4
* module/srfi/srfi-19.scm (zone-reader): handle a colon in the zone. * test-suite/tests/srfi-19.test (SRFI date/time library test): Add test. Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> --- ChangeLog v1->v2: - Improved error message to be more meaningful. - Removed duplicated code: in the v1, the read, oef-object test, and error messages were duplicated. --- module/srfi/srfi-19.scm | 16 ++++++++++------ test-suite/tests/srfi-19.test | 3 +++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/module/srfi/srfi-19.scm b/module/srfi/srfi-19.scm index 570f933ca..ad1867506 100644 --- a/module/srfi/srfi-19.scm +++ b/module/srfi/srfi-19.scm @@ -1267,12 +1267,16 @@ (list "Invalid time zone number" ch))) (set! offset (+ offset (* (char->int ch) 60 60)))) - (let ((ch (read-char port))) - (if (eof-object? ch) - (time-error 'string->date 'bad-date-template-string - (list "Invalid time zone number" ch))) - (set! offset (+ offset (* (char->int ch) - 10 60)))) + (let ((f (lambda () + (let ((ch (read-char port))) + (if (eof-object? ch) + (time-error + 'string->date 'bad-date-template-string + (list "Missing required time zone minutes" ch)) + ch))))) + (let ((ch (f))) + (if (char=? ch #\:) (set! ch (f))) + (set! offset (+ offset (* (char->int ch) 10 60))))) (let ((ch (read-char port))) (if (eof-object? ch) (time-error 'string->date 'bad-date-template-string diff --git a/test-suite/tests/srfi-19.test b/test-suite/tests/srfi-19.test index 1d56214e4..55eb82320 100644 --- a/test-suite/tests/srfi-19.test +++ b/test-suite/tests/srfi-19.test @@ -120,6 +120,9 @@ incomplete numerical tower implementation.)" (pass-if "string->date works" (begin (string->date "2001-06-01@14:00" "~Y-~m-~d@~H:~M") #t)) + (pass-if "string->date accepts ISO 8601 zones with a colon" + (begin (string->date "2024-12-31T23:59:59+01:00" "~Y-~m-~dT~H:~M:~S~z") + #t)) ;; check for code paths where reals were passed to quotient, which ;; doesn't work in Guile (and is unspecified in R5RS) (test-time->date time-utc->date date->time-utc) base-commit: 54c4753dd3f7506bee2778b36d7263b613ffd579 -- 2.41.0