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

script/image: cleanup #2886

Merged
merged 6 commits into from Aug 16, 2018
Merged

script/image: cleanup #2886

merged 6 commits into from Aug 16, 2018

Conversation

antonlacon
Copy link
Contributor

This unifies the code style used within script/image, and clarifies some comments. Two things that came up, and not addressed here, is that there is a test for powerpc64. I'm not aware of a powerpc64 target. Was this meant to be aarch64?

The second issue is that there is a line that adds a "Kodi commit: git hash" to RELEASE_DIR/RELEASE file, by way of scripts/git_version. git_version checks PKG_BUILD/VERSION for information. Kodi, at lease in my build for RPi2, does not create a VERSION file. It has a version.txt file, but that doesn't have the git hash. Checking for VERSION files in the other packages, only about 1/6 of the built packages for the contain such a file. Is this working as intended?

The changes in projects/RPi/options clean up comments only, so tossed that in here too.

scripts/image Outdated
mkdir -p $INSTALL/flash
mkdir -p $INSTALL/storage
BASELAYOUT_DIRECTORIES="etc dev proc run sys tmp usr var flash storage"
for directory in $BASELAYOUT_DIRECTORIES; do
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable isn't really necessary, the following would work fine:

for directory in etc dev proc run sys tmp usr var flash storage; do
  mkdir -p $INSTALL/$directory
done

@MilhouseVH
Copy link
Contributor

Thanks, all looks fine - just the one small nit.

zstd is also a SQUASHFS_COMPRESSION option for Generic, if you wanted to add it...

scripts/image Outdated
BASELAYOUT_DIRECTORIES="etc dev proc run sys tmp usr var flash storage"
for directory in $BASELAYOUT_DIRECTORIES; do
mkdir -p $INSTALL/$directory
done
ln -sf /var/media $INSTALL/media
ln -sf /usr/lib $INSTALL/lib
ln -sf /usr/bin $INSTALL/bin
ln -sf /usr/sbin $INSTALL/sbin

if [ "$TARGET_ARCH" = "x86_64" -o "$TARGET_ARCH" = "powerpc64" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should remove -o "$TARGET_ARCH" = "powerpc64" as it is not supported.

Same here: https://github.com/LibreELEC/LibreELEC.tv/blob/652f675e7daef4f3ea93764ccfe1ca13517c80fb/packages/virtual/initramfs/package.mk#L27

@MilhouseVH
Copy link
Contributor

The second issue is that there is a line that adds a "Kodi commit: git hash" to RELEASE_DIR/RELEASE file,

Yes, this isn't working (I wonder if it ever did). What you currently get is a RELEASE file that contains (in a Generic tar, for example):

Generic.x86_64-8.2.5
Kodi commit:

I don't find anything in the LE repo which uses/reads the RELEASE file (unless it's used by a back-end distribution process - @chewitt?) so it's possible we could drop it entirely along with all associated code including the deletion of scripts/git_version (get_pkg_version $MEDIACENTER is a more reliable alternative anyway).

If we do need to keep the RELEASE file then we can fix the broken Kodi component by replacing the call to scripts/git_version with get_pkg_version.

@antonlacon
Copy link
Contributor Author

Updated to address comments. Replaced scripts/git_version with get_pkg_version pending a decision on what to do with the RELEASE file.

scripts/image Outdated
@@ -6,11 +6,11 @@

unset _CACHE_PACKAGE_LOCAL _CACHE_PACKAGE_GLOBAL _DEBUG_DEPENDS_LIST _DEBUG_PACKAGE_LIST

. config/functions
Copy link
Contributor

Choose a reason for hiding this comment

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

Sourcing config/functions isn't required - it's sourced by config/options.

Also corrects comment typo's

Signed-off-by: Ian Leonard <antonlacon@gmail.com>
Have IMAGE_NAME check for devel version before applying default
naming.

Generate all the baselayout directories in a for loop instead of
the long list of mkdir -p one after the other.

For 64-bit builds, $INSTALL/usr is already generated for all
arches a few lines previously, so don't repeat here.

There are changes to comments for corrections and clarity.

Unifies code style: $() versus `` and spaces before ;

Signed-off-by: Ian Leonard <antonlacon@gmail.com>
Signed-off-by: Ian Leonard <antonlacon@gmail.com>
Signed-off-by: Ian Leonard <antonlacon@gmail.com>
Switch scripts/git_version for get_pkg_version out of
config/functions. Resolves being unable to report Kodi's version
in RELEASE file.

Signed-off-by: Ian Leonard <antonlacon@gmail.com>
@antonlacon
Copy link
Contributor Author

Should scripts/git_version get deleted as well? This looks like its sole use.

@MilhouseVH
Copy link
Contributor

Should scripts/git_version get deleted as well? This looks like its sole use.

Yes, I'd delete it - nothing uses it now, and it's pretty much broken anyway.

If in future we need the kind of functionality that git_version attempted to provide then adding a trivial function to config/functions which takes two arguments, the package name and the file from the build directory that contains the git rev, would be a better and more generic solution.

Signed-off-by: Ian Leonard <antonlacon@gmail.com>
@antonlacon
Copy link
Contributor Author

I believe this is squared away.

@MilhouseVH
Copy link
Contributor

Excellent, many thanks!

@MilhouseVH MilhouseVH merged commit 24aa047 into LibreELEC:master Aug 16, 2018
@antonlacon antonlacon deleted the misc-cleanup branch August 16, 2018 08:27
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

3 participants