EDCD / EDMarketConnector

Downloads commodity market and other station data from the game Elite: Dangerous for use with all popular online and offline trading tools.
GNU General Public License v2.0
988 stars 155 forks source link

Updating FDevIDs on linux #2251

Closed 3nternamehere closed 3 months ago

3nternamehere commented 3 months ago

Please complete the following information:

Describe the bug

I've built EDMC from source by editing the PKGBUILD from the AUR and when I try to run it, it fails when updating the FDevIDs with the following error: ERROR - 72370:124063827822400:72370 EDMarketConnector.__main__:2229: EDMC Critical Error: [Errno 13] Permission denied: '/usr/lib/edmarketconnector/FDevIDs'

To Reproduce

  1. Start the application on linux

Expected behavior

The application should start successfully.

Additional Information

The error also occours with all plugins disabled.

Please Confirm the Following...

Logs

EDMarketConnector-debug.log

chennin commented 3 months ago

It's most likely related to this:

Added a new updater for the FDevID Files to keep the dependency up to date without requiring a new patch version push

I can look at it later today; it's too early: my thought right now is, the package installs the files owned by root, but the updater will be run by a regular user, how could we securely allow updates?

3nternamehere commented 3 months ago

I just changed the version number and the sha256sum to match the release. It seems to me like EDMC is trying to write to /usr/lib/edmarketconnector/ which is causing the crash.

Athanasius commented 3 months ago

Yes, I'm of the opinion that trying to maintain "remote repo has a newer file" files into the install location is a mistake.

I think I might have decided:

  1. Have the primary location be in the 'user data' file path (i.e. where third-party plugins\ is).
  2. On startup if there's no files there yet, first copy the <install>\FDevIDs\ ones into that location. Then check if there's newer ones.
  3. Just use them from that user data location, to avoid all permissions issues.

Windows users will likely see this error too the next time FDevIDs has a newer version of the file(s). I know I certainly never ran EDMarketConnector.exe as an Admin level account, see: https://github.com/EDCD/EDMarketConnector/wiki/Troubleshooting#i-ran-the-program-with-quot-run-as-administrator-quot-and

chennin commented 3 months ago

Ah, so I'm probably wrong there and it does sound like an EDMC issue :)

@3nternamehere while you wait, if you don't want to stay on 5.10.x, you could run from source instead. You could also create/chown/chmod /usr/lib/edmarketconnector/FDevIDs but that is not a good solution, especially if your system has more than one user.

3nternamehere commented 3 months ago

@chennin My very hacky workaround was to symlink /usr/lib/edmarketconnector/FDevIDs to /tmp/FDevIDs and add mkdir -p /tmp/FDevIDs to the run script, but that's not something I'd advertise as well :laughing:

Rixxan commented 3 months ago

Ah the joy of things that never showed up in beta testing. I didn't think it would be an issue but I suppose I also didn't consider who owns files under Linux (as I don't have a test env for that). It appears like it should work fine on Windows (at least from my testing), but I will double check. The idea was to lessen the number of times we need to put out a patch release simply because two lines in an fdevid file were updated.

That being said, I like the ideas of putting the updated files in the same level as Plugins. I'll make that change tonight, do any of you want to be pinged when the branch is ready for testing to ensure it works as expected?

Thank you all for the report. I'll put some work into it tonight and hopefully have a fix out the door Saturday.

Athanasius commented 3 months ago

It certainly wouldn't work on my system, given this is how the install folder looks:

image and my Windows user is a 'Limited' account. Yes, I'm probably (sadly) in the minority with doing that.

3nternamehere commented 3 months ago

I'm also looking into it, but I'm not familiar with the project and also not experienced with programming in general. Most of the coding I did in the past was for data analysis, so mostly just math stuff and reading data from CSVs. But I could of course at least help with testing.

Edit: I think I've got a possible solution: in update.py line 37 replace config.respath_path with config.app_dir_path. But like I said, I don't know how the rest of the program works, so maybe there's other problems down the line or on windows.

Edit2: The alternative might be to keep EDMC as is and install the FDevIDs with the package, but then the updater would still be broken on linux.

Rixxan commented 3 months ago

I'm like 90% sure you're right, but can't test while I'm at work (darn gainful employment). If it's that simple might be able to have this out the door tonight.

Athanasius commented 3 months ago

Edit: I think I've got a possible solution: in update.py line 37 replace config.respath_path with config.app_dir_path. But like I said, I don't know how the rest of the program works, so maybe there's other problems down the line or on windows.

That will work so long as github is reachable at startup. To avoid issues with that it still needs that initial copy (hey, maybe this is where core code starts using shutils such that it's not packaged only for plugin use!) from the install location to the app_dir_path location. I think it's better to do that than force any code utilising the files to have to check both locations (even if via a helper function).

This is ... almost ... a Plugins API breaking change, but because the "distribution" copy will still be in the old location, just possibly out of date, it isn't.

3nternamehere commented 3 months ago

All I can say is, it worked for me and the FDevIDs are stored in ~/.local/share/EDMarketConnector/FDevIDs/, which is probably a more sensible location to store mutable files.

chennin commented 3 months ago

I can help test as well

Rixxan commented 3 months ago

@3nternamehere See how this works and if this progresses

2253

3nternamehere commented 3 months ago

@Rixxan This seems to work for me without problem. FDevIDs are stored in ~/.local/share/EDMarketConnector/FDevIDs.

Debug-log: EDMarketConnector-debug.log

Looking through the debug-log, this seems a bit weird to me: DEBUG - 16031:128070297003840:16031 EDMarketConnector.__main__:2080: Platform: linux False But it doesn't seem to affect the functionality on my end.

Rixxan commented 3 months ago

Deployed in 5.11.1. Considering matter resolved.