Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove need of symlinks for the repository #4254

Merged
merged 6 commits into from Mar 10, 2020

Conversation

CvH
Copy link
Member

@CvH CvH commented Mar 8, 2020

Currently we symlinking/alias addon repos for several devices.

To avoid messing around with server/jenkins configs at every repo bump and the possibility to mess it up, just hard code it into the image what should be used.

With Repo 9.80.3 there is only a single addon build for RK and AW (AW H3).
This is only a temporary solution till we have some kind of arm32 addon build that could be used by every arm device, a H3 build is what comes quite close to it.

AML could use it too but currently hold back till it has the same kernel version in use.
RPi4 basically the same, RPi 2-3 needs an more relaxed compiler options that should be included into a arm32 "project".

This is an intermediate step to find possible problems.

@CvH
Copy link
Member Author

CvH commented Mar 9, 2020

Opinions welcome :)

@MilhouseVH
Copy link
Contributor

LGTM! 😄

projects/Allwinner/options Outdated Show resolved Hide resolved
config/options Outdated Show resolved Hide resolved
Copy link
Member

@HiassofT HiassofT left a comment

Choose a reason for hiding this comment

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

looks good to me, moving ADDON_PATH/ADDON_URL up in config/options could be a good idea - people could override ADDON_PROJECT etc in their .libreelec/options files, so no strong opinion on that

@CvH CvH force-pushed the 10.0/repo_symlink branch 2 times, most recently from da9dca0 to 52784f1 Compare March 9, 2020 20:14
@CvH
Copy link
Member Author

CvH commented Mar 9, 2020

moved the block a bit, should work I guess ?

@MilhouseVH
Copy link
Contributor

Thanks, that allows anyone to override it with their personal options (same as they can override everything else).

@HiassofT HiassofT merged commit 097260a into LibreELEC:master Mar 10, 2020
@CvH CvH deleted the 10.0/repo_symlink branch March 10, 2020 20:12
@150balbes
Copy link
Contributor

After this change , overrides for add-on location parameters stopped working if they are specified in the "device/options" file . And constantly tries to add an extra parameter $ ADDON_PROJECT to the path description

@150balbes
Copy link
Contributor

150balbes commented Mar 12, 2020

transfer from

ADDON_PATH="$ADDON_VERSION/$ADDON_PROJECT/$TARGET_ARCH"
ADDON_URL="$ADDON_SERVER_URL/$ADDON_PATH"

config/options to distributions/LibreELEC/options

solved the problem , now everything works correctly

@MilhouseVH
Copy link
Contributor

What kind of override are you trying to apply? You can still rewrite the ADDON_PATH and ADDON_URL variables in your $ROOT/.libreelec/options or $HOME/.libreelec/options.

@150balbes
Copy link
Contributor

150balbes commented Mar 12, 2020

Try setting parameters in the file (projects/platform_/devices/device_1/options) and the system will ignore them or add the wrong elements.

The settings file (projects/platform_/devices/device_1/options) must have maximum priority over all other settings files.

@150balbes
Copy link
Contributor

in your $ROOT/.libreelec/options or $HOME/.libreelec/options.

I don't have these files (I don't use them)

@MilhouseVH
Copy link
Contributor

I don't have these files (I don't use them)

You can create them, use whichever you prefer - one is under git control, the other is not.

The settings file (projects/platform_/devices/device_1/options) must have maximum priority over all other settings files.

And it does, but it doesn't guarantee you can always rewrite any variable from within a device options file.

After this PR, if you want to rewrite the ADDON_PATH or ADDON_URL then you need to be a little more creative - for instance set the variables BALBES_ADDON_PATH and BALBES_ADDON_URL in your device/options file(s), then in $ROOT/.libreelec/options add:

ADDON_PATH="${BALBES_ADDON_PATH:-$ADDON_PATH}"
ADDON_URL="${BALBES_ADDON_URL:-$ADDON_URL}"

We could possibly avoid setting the ADDON_PATH and ADDON_URL vars in config/options if they are already set, ie.

[ -z "$ADDON_PATH" ] && ADDON_PATH="$ADDON_VERSION/$ADDON_PROJECT/$TARGET_ARCH"
[ -z "$ADDON_URL" ] && ADDON_URL="$ADDON_SERVER_URL/$ADDON_PATH"

as this should allow the vars to be overidden by a device/options file (totally untested). PR welcome. :)

@150balbes
Copy link
Contributor

150balbes commented Mar 12, 2020

You can create them, use whichever you prefer - one is under git control, the other is not.

Yes, i'm can create and use them .
I proceed from the minimum expediency , if the build environment itself does not create these files when working (if there are none), then their absence should not change the behavior of the build system by default ( for example, the user cloned GIT LE and tries to perform the build by default).

After this PR, if you want to rewrite the ADDON_PATH or ADDON_URL then you need to be a little more creative

Why "reinvent the wheel " and take extra steps :).
Just transfer the added change with this PR to another file and everything starts working correctly, I have already checked it. :)

Perhaps I did not explain it well, you just need to transfer the 2 lines added by this PR from the file (config/options) to the file ( distributions/LibreELEC/options).
for example. to the place where the variable was added, so that

distributions/LibreELEC/options

set the default addon project
ADDON_PROJECT="${DEVICE:-$PROJECT}"
ADDON_PATH="$ADDON_VERSION/$ADDON_PROJECT/$TARGET_ARCH"
ADDON_URL="$ADDON_SERVER_URL/$ADDON_PATH"

@MilhouseVH
Copy link
Contributor

@150balbes I don't think you've understood this change as moving that code back to where you suggest simply defeats the purpose of this change. We moved the location of where we set the ADDON_PATH and ADDON_URL variables so that it gives the device/options files an opportunity to set a single variable (ADDON_PROJECT) and then we don't need to repeatedly set ADDON_PATH and ADDON_URL in multiple locations. This change supports the standard LE use case.

There is (as I've described) a somewhat inelegant workaround for non-standard/custom/out-of-tree addon paths and urls. We could possibly continue to support device/options setting these two vars by making the setting in config/options conditional - I guess I'll have to look into that...

@MilhouseVH
Copy link
Contributor

@150balbes also, you still haven't explained what it is you are doing that changes these variables, you mentioned "adding parameters" so are you adding another variable to the end of ADDON_URL? It's entirely possible we could integrate your requirements fairly easily without you having to rewrite these variables at all.

@150balbes
Copy link
Contributor

We moved the location of where we set the ADDON_PATH and ADDON_URL variables so that it gives the device/options files an opportunity to set a single variable (ADDON_PROJECT) and then we don't need to repeatedly set ADDON_PATH and ADDON_URL in multiple locations. This change supports the standard LE use case.

An error is hidden in this processing logic . :) You wanted to get rid of duplicating addon variables , but as a result you got the need to duplicate another variable in all device\optons. What happens if I don't set anything in the device\options settings ? I will get a non- working version of the repositories.

also, you still haven't explained what it is you are doing that changes these variables, you mentioned "adding parameters" so are you adding another variable to the end of ADDON_URL? It's entirely possible we could integrate your requirements fairly easily without you having to rewrite these variables at all.

On the contrary, I remove the extra elements :), and the new logic forces me to add an extra intermediate element ( device name) again. I have been using the same repository for all ARM platforms (AW+RK+AML) for a long time without any extra DEVICE element, and all Addons work (if they work in principle for ARM). Setting up a repository http://my_libreelec/addons/9.80.3/arm/addons.xml and similarly for aarch64 (instead of arm in the settings).

@MilhouseVH
Copy link
Contributor

An error is hidden in this processing logic . :) You wanted to get rid of duplicating addon variables , but as a result you got the need to duplicate another variable in all device\optons.

It's not an error in the processing logic (at least not for standard LibreELEC) - it's a code simplification as we now only construct the ADDON_PATH and ADDON_URL in one location and don't have to duplicate how we construct ADDON_URL in multiple locations.

I accept it may not be taking into consideration every conceivable custom downstream project that wants to do things differently, but if they're changing the build system then they should be able to implement their own solution.

What happens if I don't set anything in the device\options settings ? I will get a non- working version of the repositories.

Currently you would use the default ADDON_PROJECT set by distributions/LibreELEC/options.

On the contrary, I remove the extra elements :), and the new logic forces me to add an extra intermediate element ( device name) again.

OK, so we could make ADDON_PROJECT optional, then you would just need to set ADDON_PROJECT="" in distro/options (and then remove ADDON_PROJECT from any other project or device options that might be setting it) if you don't want it.

We would need something like the following change in config/options to support that use case:

if [ -n "${ADDON_PROJECT}" ]; then
  ADDON_PATH="$ADDON_VERSION/$ADDON_PROJECT/$TARGET_ARCH"
else
  ADDON_PATH="$ADDON_VERSION/$TARGET_ARCH"
fi
ADDON_URL="$ADDON_SERVER_URL/$ADDON_PATH"

Right?

@150balbes
Copy link
Contributor

Funny, I tried to make almost the same construction for the solution (with adding an empty variable to my project), but it didn't work for me because of the wrong paramter I used. :)

if [ -z "${ADDON_PROJECT}" ]; then
ADDON_PATH="$ADDON_VERSION/$ADDON_PROJECT/$TARGET_ARCH"
else
ADDON_PATH="$ADDON_VERSION/$TARGET_ARCH"
fi

@MilhouseVH
Copy link
Contributor

If you can confirm you are happy with http://ix.io/2e34 I'll PR it later.

@150balbes
Copy link
Contributor

Added a patch and an empty variable in device/options (ADDON_PROJECT=""), checked that everything works correctly.
Tnx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants