Skip to content

Commit

Permalink
cli: include options now accept paths in next argument, enabling ~ su…
Browse files Browse the repository at this point in the history
…bstitution

old syntax "-I~/path/" -> this is ignored by the shell, so "~" is not
substituted with $HOME, because it's one string

new syntax: "-I ~/path/" -> shell will treat ~/path as individual string
and substitute the ~ (if it does that usually for you)

Also the sjasmplus will now check all include paths before starting
assembling and report if some can't be opened.

And it reports if some starts with literal "~" (fopen will not
substitute that, so if shell didn't before starting sjasmplus, report it
as error).

OLD SYNTAX STILL WORKS (and it's not going to be removed any time soon,
if ever), but docs and --help will suggest the new syntax only.
  • Loading branch information
ped7g committed Sep 16, 2024
1 parent 214e3b7 commit 43a2c8b
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 4 deletions.
14 changes: 12 additions & 2 deletions docs/documentation.xml
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,9 @@
<listitem>
<synopsis>
- <link linkend="po_incbin">`INCBIN`</link>, <link linkend="po_inchob">`INCHOB`</link> and <link linkend="po_inctrd">`INCTRD`</link> support now include paths priority (angle &lt;filename&gt; vs "filename")
- include paths which can't be opened before assembling are now reported in error message
- include paths which start with literal tilde '~' are reported in error message
- include options <link linkend="s_cli">`-i, -I, --inc`</link> can take path from next CLI argument (new recommended syntax)
- added Amstrad CPC device <link linkend="po_device">AMSTRADCPCPLUS</link> - by Laurent Carlier
- added Amstrad CPC+ <link linkend="po_savecpr">`SAVECPR`</link> to save CPR cartridge - by Laurent Carlier
- ZX Next CSpect emulator v2.19.9.1: `break` opcode changed to `FD 00`
Expand Down Expand Up @@ -471,7 +474,7 @@
--outprefix=&lt;path&gt; Prefix for save/output/.. filenames in directives
Note: if prefix is folder, it must already exist and add trailing slash. Prefix
is applied only to filenames defined in source (not to CLI arguments).
-i&lt;path&gt; or -I&lt;path&gt; or --inc=&lt;path&gt; ( --inc without "=" to empty the list)
-i &lt;path&gt; or -I &lt;path&gt; or --inc= &lt;path&gt; (--inc only to empty the list)
Include path (later defined have higher priority)
--lst[=&lt;filename&gt;] Save listing to &lt;filename&gt; (&lt;source&gt;.lst is default)
--lstlab[=sort] Append [sorted] symbol table to listing
Expand Down Expand Up @@ -510,6 +513,13 @@
listing): <code>--sym</code>, <code>CSPECTMAP</code> and <code>LABELSLIST</code>.
The <code>--msg=lstlab</code> sets always sorting "on" (no "unsorted" option).
</para>
<para>
(since v1.21.0) Include path options <code>-i, -I, --inc=</code> now accept path after space, ie.
<code>-I some/path/</code> - this is new and recommended syntax. If you get error "error: include
path starts with ~ (check docs)", it means your shell did not substitute the ~ character with
$HOME path, probably because you didn't put space between option and ~, so shell considers it
to be single string not beginning with ~. Insert space between like: <code>-I ~/include/path</code>
</para>
<para>Value for <code>--syntax</code> option may consist of multiple letters, omitting letter
for particular feature will use the default setting:
<synopsis> a Multi-argument delimiter ",," (default is ",") (but dec|inc|pop|push accept also ",")
Expand Down Expand Up @@ -555,7 +565,7 @@ label: dw 15
sjasmplus --lst --lstlab example.asm</programlisting>
will execute the assembling as if command line "<code>sjasmplus --zxnext=cspect --msg=war --lst --lstlab example.asm</code>"
was used. Known issue: parser of environment variable delimits each option on any white-space character, so
option containing space character will be incorrectly parsed (like "-Ifile-path with space" = fails and
option containing space character will be incorrectly parsed (like "-I file-path with space" = fails and
there is no way to escape/quote the path in the SJASMPLUSOPTS variable to make it work).
</para>
</section>
Expand Down
24 changes: 22 additions & 2 deletions sjasm/sjasm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ static void PrintHelpMain() {
_COUT " --i8080 Limit valid instructions to i8080 only (+ no fakes)" _ENDL;
_COUT " --lr35902 Sharp LR35902 CPU instructions mode (+ no fakes)" _ENDL;
_COUT " --outprefix=<path> Prefix for save/output/.. filenames in directives" _ENDL;
_COUT " -i<path> or -I<path> or --inc=<path> ( --inc without \"=\" to empty the list)" _ENDL;
_COUT " -i <path> or -I <path> or --inc= <path> (--inc only to empty the list)" _ENDL;
_COUT " Include path (later defined have higher priority)" _ENDL;
_COUT " --lst[=<filename>] Save listing to <filename> (<source>.lst is default)" _ENDL;
_COUT " --lstlab[=sort] Append [sorted] symbol table to listing" _ENDL;
Expand Down Expand Up @@ -562,7 +562,11 @@ namespace Options {
IncludeDirsList = new CStringsList(val, IncludeDirsList);
} else {
if (!doubleDash || '=' == arg[5]) {
Error("no include path found in", arg, ALL);
if (argv[i+1] && '-' != argv[i+1][0]) { // include path provided as next argument (after space, like gcc)
IncludeDirsList = new CStringsList(argv[++i], IncludeDirsList); // also advance i++
} else {
Error("no include path found in", arg, ALL);
}
} else { // individual `--inc` without "=path" will RESET include dirs
if (IncludeDirsList) delete IncludeDirsList;
IncludeDirsList = nullptr;
Expand Down Expand Up @@ -592,6 +596,20 @@ namespace Options {
++i; // next CLI argument
} // end of while ((arg=argv[i]) && ('-' == arg[0]))
}

void checkIncludePaths(const CStringsList* includes) {
FILE* dir;
while (nullptr != includes) {
if (FOPEN_ISOK(dir, includes->string, "r")) {
fclose(dir);
} else {
const char* errtxt = '~' == includes->string[0] ? "include path starts with ~ (check docs)" : "include path not found";
Error(errtxt, includes->string, ALL);
}
includes = includes->next;
}
return;
}
};

int parseSyntaxOptions(int n, char** options) {
Expand Down Expand Up @@ -728,6 +746,8 @@ int main(int argc, char **argv) {
Warning("missing --fullpath with --sld may produce incomplete file paths.", NULL, W_EARLY);
}

optParser.checkIncludePaths(Options::IncludeDirsList);

if (Options::ShowVersion) {
if (Options::HideLogo) { // if "sjasmplus --version --nologo", emit only the raw VERSION
_CERR VERSION _ENDL;
Expand Down
14 changes: 14 additions & 0 deletions tests/test build script and options/include_paths/tilde.asm
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
; testing multiple new changes in v1.21.0:
; - include path options (i, I, inc) now can be provided with path by next following CLI argument, ie. "-I dir/"
; (this allows the shell to expand ~/ paths to full path)
; - include path literally starting with "~" will be reported with hint to read docs (shell didn't expand it)
; - include path which can't be initially opened will be reported in error (missing dir?)
;
; (test is not testing if the shell did correctly expand "~" in normal use case, because it's not known
; how the test enviroment is set up, if it has $HOME and if it is related to running tests - leap of faith)
;
; (oh, it does not substitute it from test runner when provided in .options file any way... strange, but it's fine for this test)
;

INCLUDE "tilde.i.asm" ; verify the include path works
INCLUDE <tilde.i.asm>
37 changes: 37 additions & 0 deletions tests/test build script and options/include_paths/tilde.msglst
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
error: no include path found in: -i
error: no include path found in: --inc=
error: include path starts with ~ (check docs): ~/tilde3
error: include path not found: /dev/null/er4
error: include path not found: /dev/null/er3
error: include path not found: /dev/null/er2
error: include path not found: /dev/null/er1
error: include path starts with ~ (check docs): ~/tilde2
error: include path starts with ~ (check docs): ~/tilde1
# file opened: tilde.asm
1 0000 ; testing multiple new changes in v1.21.0:
2 0000 ; - include path options (i, I, inc) now can be provided with path by next following CLI argument, ie. "-I dir/"
3 0000 ; (this allows the shell to expand ~/ paths to full path)
4 0000 ; - include path literally starting with "~" will be reported with hint to read docs (shell didn't expand it)
5 0000 ; - include path which can't be initially opened will be reported in error (missing dir?)
6 0000 ;
7 0000 ; (test is not testing if the shell did correctly expand "~" in normal use case, because it's not known
8 0000 ; how the test enviroment is set up, if it has $HOME and if it is related to running tests - leap of faith)
9 0000 ;
10 0000 ; (oh, it does not substitute it from test runner when provided in .options file any way... strange, but it's fine for this test)
11 0000 ;
12 0000
13 0000 INCLUDE "tilde.i.asm" ; verify the include path works
# file opened: tilde/tilde.i.asm
1+ 0000 ASSERT 1
2+ 0000
# file closed: tilde/tilde.i.asm
14 0000 INCLUDE <tilde.i.asm>
# file opened: tilde/tilde.i.asm
1+ 0000 ASSERT 1
2+ 0000
# file closed: tilde/tilde.i.asm
15 0000
# file closed: tilde.asm

Value Label
------ - -----------------------------------------------------------
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-I~/tilde1 --inc=~/tilde2 -I/dev/null/er1 --inc=/dev/null/er2 -i tilde/ --inc= tilde/ -I /dev/null/er3 --inc= /dev/null/er4 -i ~/tilde3 -i --inc= --i8080
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ASSERT 1

5 comments on commit 43a2c8b

@kborowinski
Copy link
Contributor

@kborowinski kborowinski commented on 43a2c8b Sep 16, 2024

Choose a reason for hiding this comment

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

@ped7g This commit breaks ~490 test on my local build (GCC and MSVC):
image

@ped7g
Copy link
Collaborator Author

@ped7g ped7g commented on 43a2c8b Sep 16, 2024

Choose a reason for hiding this comment

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

yes, I know, it breaks also here in the CI.

But only in windows, so not sure if it's worth fixing... 🤷‍♂️

(just joking... maybe... I haven't yet looked into it, seems like quick check for directory existence by fopen("directory_name", "r") + fclose() does not work in windows, so I will have to search for some hint why is that and if there's anything possible to do about it, or if there's better way to check for include path existence)

thanks for making sure I'm aware, would be very helpful if it would not fail on github CI.

@ped7g
Copy link
Collaborator Author

@ped7g ped7g commented on 43a2c8b Sep 16, 2024

Choose a reason for hiding this comment

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

Other option is to skip path check under windows, but that means the test of the feature would have to be skipped too, etc... I would prefer to figure out how to write it in universal way. I just find very annoying the fopen of directory fails, didn't expect that, nor can I imagine any sane reason why is it like that.

@ped7g
Copy link
Collaborator Author

@ped7g ped7g commented on 43a2c8b Sep 16, 2024

Choose a reason for hiding this comment

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

ok, after quite some googling I think the least annoying option for me is just to give up and go C++17 and use std::filesystem instead of all these posix-like hacks (I somehow did remember windows being more POSIX compliant/friendly, turns out it was less than I assumed and pretty much abandoned by now).

If there wouldn't be long history of sjasmplus supporting windows, I would so just drop the support right now, there's always some issue with it whenever I develop something new, such a crap OS.

@ped7g
Copy link
Collaborator Author

@ped7g ped7g commented on 43a2c8b Sep 16, 2024

Choose a reason for hiding this comment

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

Track #240 for fix (or just track build status on github). But if the C++17 is solution, it may take some time until all settles down, meanwhile "master" on github is non-windows-only.

Please sign in to comment.