Replace / by _ in name to avoid issue when writing the cache file.

[gmsoft-tuxicoman]

Description

The latest version of homeassistant skills builds an intent file and tries to register it.
However this fails with the following error message :

FANN Error 2: Unable to open configuration file "/home/mycroft/.local/share/mycroft/intent_cache/homeassistant.mycroftai:{/tmp/mycroft/cache/HomeAssistantSkill/tracker}.intent.net" for writing.

How to test

Install the latest homeassitant skill (HEAD).

Contributor license agreement signed?

CLA (7-23-2017)

[devops-mycroft]

Hello, @gmsoft-tuxicoman, thank you for helping with the Mycroft project! We welcome everyone
into the community and greatly appreciate your help as we work to build an AI
for Everyone.

To protect yourself, the project, and users of Mycroft technologies we require
a Contributor Licensing Agreement (CLA) before accepting any code
contribution. This agreement makes it crystal clear that along with your
code you are offering a license to use it within the confines of this project.
You retain ownership of the code, this is just a license.

Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank
you!

[gmsoft-tuxicoman]

CLA (7-23-2017)

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[krisgesling]

Thanks for flagging this Guy,

Also CLA received and you've been added to our list of awesome contributors - our friendly bot will update that shortly.

Based on the error message you got I'm wondering if we should use the filename rather than entity_file that may be a full path like in this situation. It would cause an issue if a single Skill tried to register two entity files with the same name but different paths, but is that too much of an edge case to worry about?

I'm interested if others want to chime in on this too?
@JarbasAl it looks like OVOS has got the same issue.

[gmsoft-tuxicoman]

The only downside on using the complete path is that you might run into the maximum filename length on the OS.
Other that that, it completely avoids collision which might be more difficult to troubleshoot.

[forslund]

Couldn't this run into collisions with files with an actual _? would it be more correct to use url-encoding or some such in the "name"? It's sort of unlikely but a file like _tmp_thing.entity in the locale folder could get the same name as a /tmp/thing.entity file...

In addition could the same issue also exist on L1022? or is the logic different enough there to not cause this issue?

[gmsoft-tuxicoman]

Indeed this collision could happen too. Another option is to hash the filename but it makes it harder to identify the right cache file when looking it up in the directory.

[forslund]

At one point there was actually a hash but it was changed for this exact reason :)

[gmsoft-tuxicoman]

Then the solution is evident, filename + 8-16 bytes hash :)

[gmsoft-tuxicoman]

Please review the last commit. It uses basename_<full_path_hash> so that collisions are not possible.

[forslund]

I think it looks like a good idea. The lint tool thinks the line is too long however and I wonder if a specific hash from hashlib should be used instead of the hash() since the latter can change between python versions (afaik)

[gmsoft-tuxicoman]

I'm not sure it's a big deal if hash() changes between versions since those cache files gets regenerated each time.

[forslund]

I was under the impression the cache files remained, at least the .intent files are cached and reused at a later date, and the files generated by the entity-files has the same date as the corresponding .intent file as far as I can see. But I'm not 100% sure on the inner workings of Precise so I may very well be wrong.

In any case, I still think it would be nice if the files guaranteed the same hash, even if the files are generated anew on each startup a change in the hash would cause a "duplicate" file.

[forslund]

Apart from the linting issue (non-aligned followup row which needs to be fixed to satisfy the autobots) I think this looks great.

[forslund]

You should be able to check the file with flake8 locally

[gmsoft-tuxicoman]

Thanks for the tip !

[gmsoft-tuxicoman]

Anything else you need to get this merged ?

[forslund]

Seems like there are some unittests failing now. Let me know if you need help with them

[forslund]

Seems to be a tiny lint issue now :(

[gmsoft-tuxicoman]

Finally ! :)

[gmsoft-tuxicoman]

Any luck getting this merged ?

[forslund]

I think it's good to merge, @krisgesling does the Mycroft team agree?

[krisgesling]

Grrr - I resolved a merge conflict and now the Github Action is failing.

I'll need to fix that first :(

[gmsoft-tuxicoman]

@krisgesling any luck ?

[gmsoft-tuxicoman]

@krisgesling any luck merging this ?