tools/mkconstfs: Add an improved tool. by jcarrano · Pull Request #9282 · RIOT-OS/RIOT

jcarrano · 2018-06-04T08:42:33Z

Background

I need to generate a constant file system to run the Lua test suite, and rather than roll my own, I decided to enhance the one already in RIOT.

The problems I found with the current script:

It outputs to stdout only and does not buffer the whole output, meaning it can fail but still generate a partial output. When this is used inside a makefile, the result is that the first call to make fails but on the second call there is already a (partial) file up to date and make ignores the target.
It crawls the file system, recursively including everything it finds in a directory tree. This is not flexible enough.
Filename mangling is not robust enough (it can generate invalid identifier names).

Contribution description

In order not to break code that may use the older script, this PR adds a new script with a different name.

Writing to a file is now possible without having to resort to shell redirection.
The output is buffered and written only after all processing is done, so there is no partial files and no timestamp changes in case of errors.
~~Complete name mangling. It will only generate valid C identifiers.~~ No more name mangling: no more problems.
Files to be included should be given in the command line - no more crawling. There are tools to find and filter files better than any we may ever make ourselves.
Command line parsing is done by argparse.

At some point I will extend the tool to embed files in other formats different from constfs.

Issues/PRs references

I need this for #9256.

MichelRottleuthner

looks good to me in general. the nested maps and lambdas could be split up a bit though.

I tested the generated file with examples/filesystem. everything looks fine - apart from the fact that the path properties of the generated constfs_file_t structs are missing a slash at the beginning, compared to the old mkconstfs.py. This leads to the files not being accessible. With an added slash infront of the filename everything works fine.

MichelRottleuthner · 2018-06-21T16:32:30Z

dist/tools/mkconstfs/mkconstfs2.py

+                        lambda x: x[0]//16)
+                )  # noqa: E127
+            )
+        )


wooha "the python foo is strong in this one" Not sure if this is more "nice and efficient" than it is "ugly and hard to understand/maintain"^^ I'm also wondering why you are mixing yield from and itertools.chain. I'm certain this can be written in a cleaner way, which might also help with getting rid of all the #noqa ... markers ;)

actually I'm referring to the lines 105 -111

MichelRottleuthner · 2018-06-21T16:36:47Z

dist/tools/mkconstfs/mkconstfs2.py

+
+    parser.add_argument("-r", '--root', metavar="root_base_path",
+                        help="Paths on the constf will be gerated for the real"
+                        "path of the files by considering this path to be the root"


insert whitespaces at the end of the multiline strings above (i.e. after root, real, ..)

MichelRottleuthner · 2018-06-21T16:39:42Z

dist/tools/mkconstfs/mkconstfs2.py

+                             "(i.e. there is no partial output")
+
+    parser.add_argument("-r", '--root', metavar="root_base_path",
+                        help="Paths on the constf will be gerated for the real"


jcarrano · 2018-06-22T10:43:07Z

@MichelRottleuthner I just realized this script will generate wrong paths if ran in a system that doesn't use UNIX paths. Is that a problem?

bergzand · 2018-06-22T10:46:34Z

dist/tools/mkconstfs/mkconstfs2.py

+
+    parser.add_argument("-o", '--output', metavar="output_file",
+                        help="Write the output to a file instead of stdout."
+                             "The file is only written if the command is successful"


If you use type=argparse.FileType here, argparse checks the file and mode for you automatically 1.

For some reason, I didn't like FileType, but that was in the past and I think I forgot the reasons.

Now I remember, it opens the file right away. Not a good idea with mode "w+".

MichelRottleuthner · 2018-06-22T11:35:08Z

@MichelRottleuthner I just realized this script will generate wrong paths if ran in a system that doesn't use UNIX paths. Is that a problem?

Not a real problem but I'd prefer to make it as independent from the platform as possible. Its python so everyone will assume it works wherever python is available.

jcarrano · 2018-06-22T11:40:46Z

Its python so everyone will assume it works wherever python is available.

I think the same, but I don't have a windows PC now, so I won't be able to test it.

MichelRottleuthner · 2018-06-22T11:43:24Z

I have access to a windows machine here so I can help with testing if needed

jcarrano · 2018-06-27T17:34:14Z

@MichelRottleuthner Could you test it?

MichelRottleuthner · 2018-06-28T08:30:20Z

Tried it on a windows machine but it only seems to work if used with files that are not prefixed by a path (backslashes are not converted to slashes). I'll try to find time to look into a solution later today.
Another thing that I found: you implemented filename mangling to ensure valid identifiers - would it make sense to also do this for the struct names like vfs_mount_t?

jcarrano · 2018-06-28T16:05:02Z

@MichelRottleuthner Ouch! I knew I won't get the windows thing right.

There is not chance of collision with things like vfs_mount_t because the mangling always prefixes an underscore (that's a cheap fix to ensure identifiers never start with a number). I don't think it's reasonable to check the mangled name against all identifiers defined in the file (especially because we are generating identifiers). The options are either leave it as is (what are the chances of a collision?) or drop mangling entirely and generate identifiers of the form _fileXXXXX where XXXXX is some code that does not depend on the filename.

bergzand · 2018-06-28T16:12:08Z

dist/tools/mkconstfs/mkconstfs2.py

+
+
+def addroot(fname):
+    return "/" + fname if not fname.startswith("/") else fname


Is it possible to rework this to something using the python os.path module or the pathlib module to keep it OS agnostic. It might just be what is causing issues for @MichelRottleuthner

I think this won't help (for everything) because I also got backslashes in the middle of the path

Ah, so do I understand correctly that the problem is that a path provided as path\to\file on windows shows up in the constfs as /path\to\file?

Ourch, that is not supposed to happen, but it's not because of addroot.

Just had another look. The problem actually occurs only if relative paths are used. It then not only leaves backslashes there but also creates some crazy paths like /../C:\path\to/C:/path/to/file. I played around with various os.path / pathlib methods and found the following fix to make it working on linux and windows in the same way:

--- a/dist/tools/mkconstfs/mkconstfs2.py +++ b/dist/tools/mkconstfs/mkconstfs2.py @@ -1,5 +1,6 @@ #!/usr/bin/env python3 +import os import sys import argparse import pathlib @@ -50,7 +51,8 @@ static const uint8_t {varname}[] = {{ def _relpath_p(path, start): - return posixpath.relpath(path.as_posix(), start.as_posix()) + return posixpath.relpath(pathlib.Path(os.path.abspath(path)).as_posix(), + pathlib.Path(os.path.abspath(start)).as_posix())

@MichelRottleuthner Great! Can you push it to my branch, please?

just tried that but somehow it doesn't work. Did you allow edits for maintainers?

It was enabled. If it still does not work, I can apply a patch.

nope still doesn't work just add it yourself, please

MichelRottleuthner · 2018-06-28T16:18:33Z

There is not chance of collision with things like vfs_mount_t

sorry, my wording wasn't clear enough. What I meant was the name of the struct instance which is put into the constfs_name field in the C_FOOTER template. Arguably one coud blame the user for putting invalid names there but i think it would help if there is some kind of warning (or mangling).

I'll try to find time to look into a solution later today

I have to shift that to tomorrow...

jcarrano · 2018-06-28T16:51:21Z

@MichelRottleuthner I think it's out of scope to check if the user tells the tool to create a struct with an invalid name. Almost every time this tool is used it will be from within a makefile and the name will be fixed. The Correct(TM) way of generating code and making sure it's correct is to create an abstract tree and dump it as code. Templating things we will always run into some kind of corner case, and it's not practical to check everything. As long as we don't generate wrong code that still compiles and fails silently and weirdly, it's ok.

jcarrano · 2018-06-29T08:36:42Z

BTW, what is the opinion on dropping name mangling and going for serially generated identifiers? AFAIK, GCC allows for arbitrarily long identifiers, but then we could have a file that it a hairy mess of characters.

MichelRottleuthner · 2018-06-29T09:37:07Z

since the user/dev will not use the mangled names in most cases anyway you could also use short numbered identifiers. This could also save some memory ;)

MichelRottleuthner · 2018-07-02T07:27:33Z

@jcarrano squash and we are ready to go!

The new tool (mkconstfs2) features: * more robust filename handling: no need for mangling, and works on Windows. * Better output generation: nothing is written in case of failures. * Allows more control over the files that are included: - does not traverse directories, filenames must be explicitly given. - The "root" can be explicitly given (thus the tool can get the same result independently of the CWD). Thanks to MichelRottleuthner for making it work with Windows paths.

jcarrano · 2018-07-02T08:16:26Z

@MichelRottleuthner Squashed and edited the commit msg to give you some credit.

jcarrano · 2018-07-02T08:16:55Z

Oops, accidentally hit "close"!!!

jcarrano force-pushed the new-mkconstfs branch from bed320b to 7c872c2 Compare June 4, 2018 08:42

jcarrano added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tools Area: Supplementary tools Area: fs Area: File systems labels Jun 5, 2018

tcschmidt requested a review from MichelRottleuthner June 11, 2018 09:16

danpetry mentioned this pull request Jun 11, 2018

WIP: Lua tests #9256

Closed

jcarrano requested review from jia200x and jnohlgard and removed request for MichelRottleuthner June 14, 2018 13:30

MichelRottleuthner requested changes Jun 21, 2018

View reviewed changes

jcarrano removed request for jia200x and jnohlgard June 22, 2018 10:36

bergzand reviewed Jun 22, 2018

View reviewed changes

bergzand reviewed Jun 28, 2018

View reviewed changes

MichelRottleuthner approved these changes Jul 2, 2018

View reviewed changes

jcarrano force-pushed the new-mkconstfs branch from ccb6641 to 6cfafc8 Compare July 2, 2018 08:15

jcarrano closed this Jul 2, 2018

jcarrano reopened this Jul 2, 2018

MichelRottleuthner added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 2, 2018

MichelRottleuthner merged commit 2ac61b7 into RIOT-OS:master Jul 2, 2018

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

jcarrano deleted the new-mkconstfs branch November 8, 2018 15:19



		def addroot(fname):
		return "/" + fname if not fname.startswith("/") else fname

Conversation

jcarrano commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Contribution description

Issues/PRs references

Uh oh!

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcarrano commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichelRottleuthner commented Jun 22, 2018

Uh oh!

jcarrano commented Jun 22, 2018

Uh oh!

MichelRottleuthner commented Jun 22, 2018

Uh oh!

jcarrano commented Jun 27, 2018

Uh oh!

MichelRottleuthner commented Jun 28, 2018

Uh oh!

jcarrano commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichelRottleuthner commented Jun 28, 2018

Uh oh!

jcarrano commented Jun 28, 2018

Uh oh!

jcarrano commented Jun 29, 2018

Uh oh!

MichelRottleuthner commented Jun 29, 2018

Uh oh!

MichelRottleuthner commented Jul 2, 2018

Uh oh!

jcarrano commented Jul 2, 2018

Uh oh!

jcarrano commented Jul 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcarrano commented Jun 4, 2018 •

edited

Loading