epics-containers / ibek-support

ibek definitions
Apache License 2.0
1 stars 9 forks source link

Ibek j20 changes #3

Closed gilesknap closed 11 months ago

gilesknap commented 11 months ago

This is a major refactor to tidy up ioc compilation in epics-containers

See related changes in the first test ioc here https://github.com/epics-containers/ioc-adsimdetector/pull/1

gilesknap commented 11 months ago

@coretl its getting to the point where I am happy with it.

I think we need a 'add block to file' function which would make the changes here https://github.com/epics-containers/ibek-support/blob/ibek-j20-changes/ADSupport/install.sh#L26-L62 in an idempotent fashion.

GDYendell commented 11 months ago

Public interface is python functions only in support.py and we ditch the bash

Including ditching the <support module>/install.sh scripts?

gilesknap commented 11 months ago

I like 1.

and that is what we currently have, although this one is a bit of a stretch to say it is bash 🙂

Call functions in support.py

function support {

Make RELEASE files to point to the correct paths by adding RELEASE.local

python ${GLOBALS_DIR}/support.py ${@}

}

coretl commented 11 months ago

Public interface is python functions only in support.py and we ditch the bash

Including ditching the <support module>/install.sh scripts?

I meant ditch the bash functions... install.sh would remain

gilesknap commented 11 months ago

@GDYendell @coretl We have decided to ditch functions.sh and go pure python.

Custom steps inside of install.sh instances would use bash commands if appropriate but can also call python,

gilesknap commented 11 months ago

OK @coretl @GDYendell the yet another refactor is complete.

There is now no functions.sh and all functions have been moved into ibek.

See the related PRs at:

https://github.com/epics-containers/ibek/pull/107 To see how the support functions have been integrated into ibek

https://github.com/epics-containers/ioc-adsimdetector/pull/1 To see what a Dockerfile for a generic IOC looks like now

In this repo you will want to look at the install.sh files for each support module. In particular I realized that the same CONFIG_SITE changes were required in ADCore and ADSimDetector. These changes are really at the generic IOC level rather than at the support module level. Therefore I have provided a mechanism for supplying parameters from the IOC (see https://github.com/epics-containers/ioc-adsimdetector/blob/ibek-j20-changes/generic_config_AD/config_site)

gilesknap commented 11 months ago

Note I still need to add tests for the ibek support features.

gilesknap commented 11 months ago

@coretl @GDYendell

After the latest changes we have a pretty neat Dockerfile

The most basic install.sh is tidy.

The whole build of ADSimDetector generic IOC container takes under 4 mins. so I'm voting for keeping ADCore build in the individual Generic IOCs. This includes all the optional ADCore features except GraphicsMagic but no use of ADSupport.

The final bit to do is tidy up the apt installs. Here is ADCore install.h as it stands.

I'm wondering if there is any benefit in having ibek functions that just call back out to bash. But having said that I thought that:-

Therefore I argue that I might as well add:

coretl commented 11 months ago

After the latest changes we have a pretty neat Dockerfile

Excellent, is ibek-support/_global/global.sh still the source of global patches, or is that moved to ibek now?

The most basic install.sh is tidy.

That looks very neat

The whole build of ADSimDetector generic IOC container takes under 4 mins. so I'm voting for keeping ADCore build in the individual Generic IOCs. This includes all the optional ADCore features except GraphicsMagic but no use of ADSupport.

That makes sense

The final bit to do is tidy up the apt installs. Here is ADCore install.h as it stands.

Yes, and we don't do composition of the runtime stage apt installs at the moment...

I'm wondering if there is any benefit in having ibek functions that just call back out to bash. But having said that I thought that:-

* I could have `ibek support apt-install` take a list that includes `http:// urls` and knows to wget them first.

* We need to declare what apt installs are required in the runtime stage and and ibek command for that could collect the list from each support module and then perform the install in the final stage.

Therefore I argue that I might as well add:

* ibek support apt-install [list of pakcages / http addtesses]

* ibek support apt-install-runtime [list of pakcages / http addtesses]

That sounds reasonable, but there are likely to be some that are installed at dev and runtime (like the hdf ones), so how about:

GDYendell commented 11 months ago

Are the boilerplate install script for support modules and put patch commands here if needed sections intended to be removed in actual implementations? They aren't true when it is complete.

gilesknap commented 11 months ago

@GDYendell no the last thing on my list is to modify the install.sh to not say boilerplate et al.

I will probably place a boilerplate one in _globals as something to start from when adding new support to ibek-support.

My other item is to sort the help sensibly.

Other than that I'm happy with this for now. Unless someone has objections I'm going to merge later today after the above changes.

GDYendell commented 11 months ago

no the last thing on my list is to modify the install.sh to not say boilerplate et al.

OK that sounds good. I would be tempted to say don't put them in the template either because people won't remove them. Or not have a template and point at the simplest real example we have as the starting point instead.

gilesknap commented 11 months ago

how about

#!/bin/bash

##########################################################################
# Template for support module install.sh scripts. Copy this script to a
# folder with the name of the support module. REMOVE THIS BLOCK. 
# Change the name to install.sh.
# Make appropriate changes where you see TODO below.
##########################################################################

##########################################################################
##### install script for TODO support module #############################
##########################################################################

# ARGUMENTS:
#  $1 VERSION to install (must match repo tag)
VERSION=${1}
NAME=TODO

set -xe

ibek support apt-install TODO TODO

ibek support git-clone ${NAME} ${VERSION}
ibek support register ${NAME}
ibek support add-libs TODO TODO
ibek support add-dbds TODO.dbd TODO.dbd

CONFIG='
TODO - block of text to add to CONFIG_SITE
'

ibek support add-to-config-site ${NAME} "${CONFIG}"

ibek support compile ${NAME}
ibek support generate-links ${NAME}
coretl commented 11 months ago

I think I prefer @GDYendell's idea of pointing to an existing relatively simple one, maybe asyn?

gilesknap commented 11 months ago

OK

gilesknap commented 11 months ago
  • ibek support apt-install [list of package names / addresses] install now and put in the runtime install list
  • ibek support apt-install --only dev [list of package names / addresses] install now
  • ibek support apt-install --only run [list of package names / addresses] put in the runtime install list

I like this.

gilesknap commented 11 months ago

Right - had a bit of a hiccup with a trivial problem that required rebuild of all container stages. Because it was an interaction between the dev and run stages. So wasted too much time on that:

    if only is AptWhen.run or AptWhen.both:
        add_list_to_file(RUNTIME_DEBS, debs)

Is NOT the same as:-

    if (only is AptWhen.run) or (only is AptWhen.both):
        add_list_to_file(RUNTIME_DEBS, debs)

DOH! maybe I need some sleep.

Anyway @coretl @GDYendell I feel ready to merge.

Let me know what you think. Here is what ADCore install.sh now looks like. And here is the Dockerfile for ioc_adsimdetctor

I kinda like it.

Thanks for iterating on this with me, Both. Its been very useful (my first attempt at an install.sh was crap - but useful for starting the discussion)

Regarding global.sh - its not currently doing anything and I will look at it again when I get to RTEMS. I think its nice to have a place where you can change global patching without rebuilding ibek so maybe ibek should call a script ... no sure.

GDYendell commented 11 months ago

I think this looks really good now!

I am a bit confused about ibek support apt-install --runtime vs ibek support apt-install --only=run. The latter configures the packages to be installed and also installs it into the current running container and the former pulls packages from the file to install, is that right? Are these mutually exclusive flags or might they be used together? Is the intention that ioc container Dockerfile might run RUN ibek support apt-install ... with some explicit packages, rather than just reading the runtime packages file?

gilesknap commented 11 months ago

I think this looks really good now!

I am a bit confused about ibek support apt-install --runtime vs ibek support apt-install --only=run. The latter configures the packages to be installed and also installs it into the current running container and the former pulls packages from the file to install, is that right? Are these mutually exclusive flags or might they be used together? Is the intention that ioc container Dockerfile might run RUN ibek support apt-install ... with some explicit packages, rather than just reading the runtime packages file?

They are not mutually exclusive but you would normally only use --runtime in the runtime section.

ibek support apt-install --runtime mypackage1 mypackage2

would install all the things that had previously been declared with a --run (or --both - the default) plus the two mypackage?

`ibek support apt-install --run --runtime xxx

would install all the packages declared, plus xxx plus it would add xxx to the runtime list (not something you want to do really but its sort of consistent right? ??? )