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

import current 9front rc #558

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

import current 9front rc #558

wants to merge 1 commit into from

Conversation

grobe0ba
Copy link

Imports the most recent 9front rc.

Some of the diff is reformatting, and you might choose not to merge simply because of this.
If this an issue, I can work it over again minus that.

@grobe0ba grobe0ba closed this Jul 25, 2022
@grobe0ba grobe0ba reopened this Jul 25, 2022
@dancrossnyc
Copy link
Collaborator

Perhaps you could squash all of this down into a single commit, but more importantly, could you outline the major changes from stock rc, and why importing this would be useful?

@grobe0ba
Copy link
Author

I'm happy to squash the commits down, I just didn't think about it as I was going along.

As for the why, 9front's rc contains a whole slew of bug fixes, lexer/interpreter fixes, globber fixes, memory leaks, heredoc fixes, along with a couple of extensions to the syntax. Another big point is recent work by cinap_lenrek to increase portability. To get a proper feel for everything that's been done, you'd really have to take a look at the 9front commit logs. It's quite a bit, and I'm not particularly qualified to comment on it.

The rc currently in plan9port is fully functional, but with the work that's gone into 9front's rc, I think it's worth importing. I wouldn't push for it to be in 9legacy, because the goals there are different, but I think keeping plan9port up-to-date with an currently evolving Plan 9 is a good thing.

I'll have no hard feelings if you decide this isn't something you want. I just thought I'd make it available.

@grobe0ba grobe0ba force-pushed the master branch 2 times, most recently from 91ecfb8 to be2ce6d Compare July 27, 2022 18:01
@dancrossnyc
Copy link
Collaborator

I think that's reasonable, but it's enough of a departure that it probably makes sense to get some more folks's input. In the meanwhile, rephrasing your commit message to read, rc: import latest from 9front and including more or less the text above would be really appreciated. Thanks!

@grobe0ba grobe0ba force-pushed the master branch 2 times, most recently from 90316f6 to 050f71c Compare August 2, 2022 16:44
@oridb
Copy link
Contributor

oridb commented Aug 2, 2022

Note -- this drops the recentish parser rewrite that p9p will want (and which we should probably import into 9front): 47d4646, 7d6a248, ff74f7c, 601e07b

I think this PR should be updated to incorporate them (and the 9front rc should probably be changed to parse the same dialect of rc) before we land this.

Also, as mentioned on grid, this code was run through an autoformatter -- don't do that! that makes any merges with future 9front changes will be hellish.

@grobe0ba
Copy link
Author

grobe0ba commented Aug 2, 2022

Undoing the formatting now. I'll push again once that is complete.

I hadn't realized p9p's parser had undergone as many changes as it had when I embarked on this. After discussion with @oridb, I agree that even after undoing the formatting changes, this shouldn't be merged without work on the parser.

@dancrossnyc
Copy link
Collaborator

Oh. Yes, indeed.

@grobe0ba grobe0ba force-pushed the master branch 3 times, most recently from 198e1de to 9f420c3 Compare August 2, 2022 17:59
@grobe0ba
Copy link
Author

grobe0ba commented Aug 2, 2022

Okay, this now uses the p9p parser with the 9front lexer updates.
Since we don't have a test suite, I'm unsure if there will be any difficulties with this, but it seems to function fine in casual use.

@dancrossnyc
Copy link
Collaborator

I think we're going to need more testing here. @oridb seemed to imply these changes (or at least, the basic grammar) was bound for 9front. Is that the case?

Includes work to increase portability on *nix systems by cinap_lenrek,
some extensions to the syntax, and multiple bug fixes to:
 - lexer/interpreter
 - globber
 - heredoc
and much more.
@oridb
Copy link
Contributor

oridb commented Aug 2, 2022

Let's leave this open and figure it out. I'm not opposed to importing into 9front, but would want to talk it over and with cinap before I touch that.

We've got a hackathon coming up (starting next week), so this is good timing. Let's decide what to do then.

@dancrossnyc
Copy link
Collaborator

SGTM. I'm out of the country next week, but should have Internet access.

@sfyatee
Copy link

sfyatee commented Jun 18, 2023

Any update on this? I got this patch to compile on MacOS by adding #undef NSIG in rc.h and #include <u.h> #include <signal.h> in unix.c. However in the install process it spits out this error:

9 rc ./manweb
/dev/fd/0:1: 9: No such file or directory
mk: 9 rc ./manweb  : exit status=exit(1)

Entering RC also doesn't seem to be picking up or setting the path correctly.

% 9 ls
/dev/fd/0:1: 9: No such file or directory
% acme
/dev/fd/0:2: acme: No such file or directory

What's weird is that Bash and Fish shell can execute scripts with '9' such as 9 ls and 9 lc. Any thoughts on how to resolve this issue?

@grobe0ba
Copy link
Author

This patch still has seen effectively no testing. I'm going to try to take some time over the next couple weekends to re-port it back to 9front, and see if it can get merged there.

I'm not sure what will happen here after that; presumably if it gets some use testing and lands on 9front we'll be able to get a merge here.

MacOS problems:
I can't replicate any of the issues you're having on Linux.
It looks to me like there is something wrong with your compiler or preprocessor. signal.h is included in rc.h. unix.c doesn't actually rely on plan9port, as it was written separately from plan9port, so including u.h and libc.h is wrong.

#ifdef Plan9
#include <u.h>
#include <libc.h>
#define NSIG    32
#define SIGINT  2
#define SIGQUIT 3
#else
#include <stdlib.h>
#include <stdarg.h>
#include <unistd.h>
#include <string.h>
#include <signal.h>
#ifndef NSIG
#define NSIG 32
#endif
#endif

NSIG is only defined on Plan9, or if you're on another OS and it isn't yet defined. I have no idea exactly where you're putting #undef NSIG, but it's wrong no matter where you put it.
I'm willing to take a look at the full compiler output to see if anything stands out. Please change the top of src/cmd/rc/mkfile to

<$PLAN9/src/mkhdr

CC=9c -Wall -pedantic
TARG=rc

I'll only need the output from cd $PLAN9/src/cmd/rc && mk clean && mk. You also haven't mentioned what compiler you're using. Is this an Apple compiler compiler chain, an external clang, external gcc, etc. What version of MacOS? What architecture?

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