Conversation
f4655ee to
282f6aa
Compare
gustavosr8
left a comment
There was a problem hiding this comment.
I think that 282f6aa could be more atomic, with one commit per IOC. What do you think?
db7589e to
a669638
Compare
ericonr
left a comment
There was a problem hiding this comment.
Commit message/changelog nits:
- don't capitalize after the colon. Do "base: new method..."
This implementation was introduced due to the need to build IOCs
using only internal modules from epics-in-docker. With this new
approach, building this type of IOC requires only the definition of
the required libraries and database files in the YAML configuration
file.
As a consequence, IOCs that are currently built together with the
modules will be removed in the future.
As a consequence, IOCs that are currently built together with the
modules and exported with the 'no-build' target will be removed in the future.
During development, it was identified that the direct use of the
DBD, LIBS, and DB variables was not feasible, as it caused variable
conflicts during the make execution.
Initially, the use of the "sed" command was considered to replace
and inject parameters into the Makefile; however, after further
discussion, this approach was deemed unsustainable in the long term.
Therefore, a static Makefile approach was adopted, simplifying
maintenance.
The creation of a new Dockerfile was also evaluated. However, after
analyzing the root Dockerfile, it was observed that adding a single
command line was sufficient to address the issue, avoiding code
duplication and preserving compatibility with the static-link and
dynamic-link targets.
the current Dockerfile, it was noticed that adding a single RUN statement that removes the ... was enough to address the issue. This avoids code duplication and preserves compatibility with the...n
When IOC_AREADETECTOR is set to true, the ADCore commonDriverMakefile
is included, enabling all areaDetector plugins.
is set to 'true'
images: Change in IOC build process
images: switch the IOC image build process.
Docker Compose files (mca, motorpaker, motorpigcs2 and opcua)
were restructured to support the new IOC build method.
Docker Compose files for existing IOC images (...) were restructured to use the new IOC build method.
The docker-compose-mca.yml file was replaced by
docker-compose-mca-amptek.yml, since only Amptek modules
were added to the IOC build.
Is this ok? Do we have other users? This is missing as a breaking change from the changes files. The commit should explain a bit more why this is ok.
This commit also needs to explain that moving from no-build to dynamic-link is intentional.
CHANGES.md
Outdated
| * images: Change in IOC build process. by @guirodrigueslima in | ||
| https://github.com/cnpem/epics-in-docker/pull/133 |
There was a problem hiding this comment.
This is a breaking change due to the executable paths and should be documented as such.
It also reminds me that we had discussed doing something similar to what @henriquesimoes did for Aravis? Do we still want to? Does it fit here?
|
Note: I don't want to block this PR on further changes, so I think the changelog should also mention that for now this build method is internal and only to be used by IOCs in the project itself. That way we can perform any breaking changes without worry. |
3db4632 to
022f0db
Compare
The motorParker module was updated to a new tag, adding support for the OEM controller. The Parker IOC build was removed due to the new IOC build support (#133).
The motorPIGCS2 module was updated to a new tag, adding support for the E-727 and C-887 controllers. The pigcs2 IOC build was removed due to the new IOC build support (#133).
| * The changes in the internal IOC build process in epics-in-docker affect | ||
| the directory paths and binary locations within the images. |
There was a problem hiding this comment.
I think it would be nice to document these new paths in the image list in the README, so that we can simply point people to that documentation, instead of having to re-explain it in the changelog. What do you think?
gustavosr8
left a comment
There was a problem hiding this comment.
Suggestion for 7ca4251 commit message, just rearranging some words
base: new method for IOC build
This implementation was introduced due to the need to build IOCs using
only internal modules from epics-in-docker. With this new approach,
defining the necessary libraries and database files in the YAML
configuration file is all that is required to build this type of IOC.
As a consequence, IOCs that are currently built together with the
modules and exported with the no-build target will be removed in the
future.
Initially, the use of the sed command was considered to replace
and inject parameters into the Makefile; however, after further
discussion, this approach was deemed unsustainable in the long term.
Therefore, a static Makefile approach was adopted, simplifying
maintenance.
During development, it was identified that the direct use of the
DBD, LIBS, and DB environment variables was not feasible, as it caused
variable conflicts during the make execution. This was solved by using the
'IOC_' prefix.
When IS_IOC_AREADETECTOR is set to true, the ADCore
commonDriverMakefile is included, enabling all areaDetector plugins.
It was also noticed that adding a single RUN statement in the current
Dockerfile was enough, avoiding code duplication and preserving
compatibility with the static-link and dynamic-link targets.
The Dockerfile build copies all contents from the current working
directory before running lnls-create-ioc. This is still necessary
because the IOC creation script expects to generate the entire IOC
structure from internal modules and to copy additional files into the
new IOC (such as .db, .proto, .cmd, .yaml, etc.). At the end of the
structure creation process, these files are automatically removed.
b40b7fc to
2ac1763
Compare
The motorParker module was updated to a new tag, adding support for the OEM controller. The Parker IOC build was removed due to the new IOC build support (#133).
The motorPIGCS2 module was updated to a new tag, adding support for the E-727 and C-887 controllers. The pigcs2 IOC build was removed due to the new IOC build support (#133).
ericonr
left a comment
There was a problem hiding this comment.
All IOCs built with the new build method are using no dynamic links, the
are using dynamic linkage
2ac1763 to
daf0bf7
Compare
The motorParker module was updated to a new tag, adding support for the OEM controller. The Parker IOC build was removed due to the new IOC build support (#133).
The motorPIGCS2 module was updated to a new tag, adding support for the E-727 and C-887 controllers. The pigcs2 IOC build was removed due to the new IOC build support (#133).
base/lnls-create-ioc.sh
Outdated
| if [ -d "$cmd" ]; then | ||
| cp -r "$cmd" "iocBoot/ioc$IOC_NAME" | ||
| else | ||
| cp "$cmd" "iocBoot/ioc$IOC_NAME" | ||
| fi |
There was a problem hiding this comment.
-r can be passed for files as well, so you can have a single case.
Should we support globs in the file/directory names so the lists can be shorter? Did you try that?
There was a problem hiding this comment.
I really don't know, you can decide about that
There was a problem hiding this comment.
@henriquesimoes and @gustavosr8
For context, supporting globs would make sense if we want to copy a bunch of files from iocBoot into the new IOC.
I'm pending towards not installing any file unless it's an explicitly reusable snippet that can be sourced or used some other way. Therefore, we would install very few files, and have no need for globbing. What do you think?
There was a problem hiding this comment.
I'm pending towards not installing any file unless it's an explicitly reusable snippet that can be sourced or used some other way.
@guirodrigueslima, is that true for the files from iocBoot you've proposed to include?
Therefore, we would install very few files, and have no need for globbing. What do you think?
That sounds good for me. We at least make a conscious decision of what we should inherit from upstream this way.
There was a problem hiding this comment.
I'm going to make a decision here: example files shouldn't be included in the images. We can instead link to the "IOC reference" or "IOC usage example" as part of the documentation for it in the README.
|
@henriquesimoes and @gustavosr8 if you could take an updated look :) |
daf0bf7 to
30fb754
Compare
|
Just emphasizing #133 (comment). Besides this, lgtm! |
30fb754 to
17bfc65
Compare
| \${PROD_NAME}_DBD += pvxsIoc.dbd | ||
| \${PROD_NAME}_DBD += reccaster.dbd | ||
| \${PROD_NAME}_DBD += asSupport.dbd | ||
| \${PROD_NAME}_DBD += caPutJsonLog.dbd |
There was a problem hiding this comment.
Since we are choosing the JSON version, don't #137 need to be addressed first?
| args: | ||
| APP_DIRS: /opt/epics/modules/opcua | ||
| RUNDIR: /opt/epics/modules/opcua/iocBoot/iocUaDemoServer | ||
| RUNTIME_PACKAGES: libxml2 libssl3 |
There was a problem hiding this comment.
Why did you drop these RUNTIME_PACKAGES? ldd(1) tells me the IOC binary still depends on the libxml2.
You should make a standalone commit dropping the libssl3 before converting to the new build format. This commit should document that we no longer need this library, because the distributed module was built without crypto support in Debian 12. This is something we forgot to do when upgrading to Debian 12, so you can add the following trailer in the commit message:
Fixes: 39bb0d4 (base: update to Debian 12., 2024-10-31)
There was a problem hiding this comment.
I think the libssl3 fix should be a standalone commit, as I mentioned in my comment above.
It is not related to your proposed changes, i.e. it doesn't depend on your changes to be fixed.
This implementation was introduced due to the need to build IOCs using only internal modules from epics-in-docker. With this new approach, defining the necessary libraries and database files in the YAML configuration file is all that is required to build this type of IOC. As a consequence, IOCs that are currently built together with the modules and exported with the no-build target will be removed in the future. Initially, the use of the sed command was considered to replace and inject parameters into the Makefile; however, after further discussion, this approach was deemed unsustainable in the long term. Therefore, a static Makefile approach was adopted, simplifying maintenance. During development, it was identified that the direct use of the DBD, LIBS, and DB environment variables was not feasible, as it caused variable conflicts during the make execution. This was solved by using the 'IOC_' prefix. When IS_IOC_AREADETECTOR is set to true, the ADCore commonDriverMakefile is included, enabling all areaDetector plugins. It was also noticed that adding a single RUN statement in the current Dockerfile was enough, avoiding code duplication and preserving compatibility with the static-link and dynamic-link targets. The Dockerfile build copies all contents from the current working directory before running lnls-create-ioc. This is still necessary because the IOC creation script expects to generate the entire IOC structure from internal modules and to copy additional files into the new IOC (such as .db, .proto, .cmd, .yaml, etc.). At the end of the structure creation process, these files are automatically removed. The libevent-pthreads-2.1-7 library is now installed by default in the Dockerfile, since all IOCs that support PVXs require this package. This change avoids installing the library through the RUNTIME_PACKAGES variable.
bf16ba9 to
0e3fd9d
Compare
The RUNTIME_PACKAGES variable has been removed from the docker-compose-epics-base.yml file. The libevent-pthreads-2.1-7 library is now installed natively in the Dockerfile. Docker Compose files for existing IOC images (mca, motorpaker, motorpigcs2 and opcua) were restructured to use the new IOC build method. In the docker-compose-opcua.yml file, the installation of the libssl3 library was removed since it is no longer required. The distributed module was built without crypto support in Debian 12, making this dependency obsolete. This change also addresses an oversight from the Debian 12 upgrade. Fixes: 39bb0d4 (base: update to Debian 12., 2024-10-31) The docker-compose-mca.yml file was replaced by docker-compose-mca-amptek.yml, Since only Amptek modules are used by customers in our facilities, we may be able to export new modules in the future. All IOCs built with the new build method are using no dynamic links, the main reason for using this target is to keep the module folders and their corresponding database files separate.
0e3fd9d to
d14f795
Compare
The motorParker module was updated to a new tag, adding support for the OEM controller. The Parker IOC build was removed due to the new IOC build support (#133).
The motorPIGCS2 module was updated to a new tag, adding support for the E-727 and C-887 controllers. The pigcs2 IOC build was removed due to the new IOC build support (#133).
Hello everyone,
I am opening this pull request to introduce the new IOC build method.
With this new implementation, it will no longer be necessary to build example IOCs that are part of the modules included in the base.
I am attaching this initial version and welcome suggestions from everyone regarding improvements and new features. The pull request is organized into two commits: New method for IOC build and Change in IOC build process.
I would like to emphasize the importance of moving forward with this pull request, as there are other pending tasks that depend directly on this implementation. 😄