file_packager files if source pathname contains @ sign

[alshamma]

Our CI system generates pathnames with @ in them. But, the file_packager uses @ in the --preload command line arg. The file_packager is not able to handle this case. E.g, we end up with args like:
--preload {root_path}@{folder_path}Required@/data/Required

My suggested fix is to change this line of code to use rfind instead of find. I have tested and verified this fix in our CI system.

emscripten/tools/file_packager.py

Line 240 in 1a623ed

at_position = arg.replace('@@', '__').find('@')

[sbc100]

Sounds reasonable to me. I suppose this assumes that the target of the preload doesn't contains a @ but that seems like a reasonable assumption. i.e. We still wouldn't be able to support --preload{root_path}@{folder_path}Required@{target_root}@{target_path}/Required

[alshamma]

We don't need to support @ in the target portion of preload, just in the source portion. So that restriction is okay.

[sbc100]

Oh wait, it looks like we already support @ via escaping:

      # '@@' in input string means there is an actual @ character, a single '@'                      
      # means the 'src@dst' notation.    

Can you use that method to escape them?

[alshamma]

Not readily. The source path is provided to us at build time in ninja. Might be possible if we wrote a wrapper script instead of invoking file_packager directly, but I would be reluctant to add that step.

[sbc100]

Just trying to understand here: It seems like you are constructing <foo>@<bar> from the local path foo and the target path bar. Can you not make that construction a little more involved by doing something like escape(<foo>)@<bar>? Is the process that is building the <foo>@<bar> argument somehow limited or not under your control?

Of course we could flip the logic to allow unescaped @ in foo over the current state which is to allow unescaped @ in bar, but I would also like to understand why the existing escaping mechanism that was built in doesn't work for you.

[alshamma]

Ah, our actual config file for generating the build contains '--preload', '<(PRODUCT_DIR)/Required@/data/Required'.

The PRODUCT_DIR is the full path provided by CI. CI is generating the <foo>@<bar>. We don't control that in our project code. I should have been more clear about that.

[sbc100]

And you can't do something like '--preload', '$(escape_string(<(PRODUCT_DIR))/Required@/data/Required' ? I.e. you can't inject come kind of logic or script in there?

[alshamma]

No, we don't have an escape mechanism. I'd have to write a separate python file to invoke file_packager.

Would it make sense to provide new args --preload-source and preload-target instead of using @?

[sbc100]

I think your suggested change to file packager should be harmless enough. I was mostly curious why its wasn't easy to do the escaping on your end, but I guess not easy in cmake.

[sbc100]

Fix is in #15980

[kripken]

I am somewhat concerned about this being a breaking change. It's hard to tell if there are current users that depend on the behavior we have right now, but it's possible.

It seems symmetrical to support an unescaped @ in either the source or the dest, so making this change would flip us from one option to a comparable one, unless I'm missing something? If the proposed option is more natural somehow then I could see that as an argument for a breaking change, but I'm not sure I see that?

Alternatively, --preload-source / --preload-target as suggested above seems like a safe thing to add (at the cost of more options, but they would just be sugar for an existing option, so that does not worry me).

[sbc100]

Adding more arguments, especially ones which are inherently linked together doesn't seem great to me.

How about a third way: We introduce another possible separator ::. This is what wasmtime uses to map files: (https://github.com/bytecodealliance/wasmtime/blob/main/docs/WASI-tutorial.md)

$ wasmtime --dir=. --mapdir=/tmp::/var/tmp demo.wasm test.txt /tmp/somewhere.txt

This would only break if somebody wanted to include a file with two colon in the name... which seems likely.. but I guess not impossible. Since : is illegal in windows it seems especially unlikely.

[kripken]

Interesting idea. I like the possible convergence of conventions across tools. However, a second separator still keeps the risk of breakage of existing users. Perhaps the windows argument has some weight, but I would not be surprised if many of our devs use only mac or linux. Sadly I checked and :: is a valid filename on linux at least.