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

base: add NDSSCPimega areaDetector plugin. #31

Merged
merged 3 commits into from
Nov 29, 2023
Merged

base: add NDSSCPimega areaDetector plugin. #31

merged 3 commits into from
Nov 29, 2023

Conversation

henriquesimoes
Copy link
Collaborator

This plugin computes the geometric restoration for Pimega detectors based on the LNLS ssc-pimega library, and allows Pimega IOC to provide coherent data to users.

Even though ADPimega is not yet supported (#15), this plugin can still be used with ADSimDetector for debugging purposes. Furthermore, it makes it possible to build ADPimega (and its IOC) out of epics-in-docker, but still using NDSSCPimega from base.

This is now possible to be merged due to the refactoring I've done to use ssc-pimega from GitHub's repository. To reuse the built library in the runtime image, I've included the copy of /usr/local/lib.

@henriquesimoes henriquesimoes added the enhancement New feature or request label Nov 8, 2023
base/.env Outdated Show resolved Hide resolved
Dockerfile Outdated
Comment on lines 36 to 40
COPY --from=build-image /usr/local/lib /usr/local/lib
RUN ldconfig

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if this is the best place to put it. I'm thinking about we applying an optimization afterwards, which will be applicable only for no-build and dynamic-link. But as of now, it is technically possible be moved to base (and also be applied to static-link), which avoids duplicated configuration.

Maybe it is time to create another shared stage for no-build and dynamic-link(?) This has some drawbacks, such as being required to duplicate packages in {BUILD,RUNTIME}_TAR_PACKAGES once again (due to using COPY --from=build-image in this shared stage).

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean something like dynamic-base? I don't know if it's worth it.

Copy link
Collaborator Author

@henriquesimoes henriquesimoes Nov 23, 2023

Choose a reason for hiding this comment

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

It could be. I don't know if it's worth either.

A relevant point I now see is that it might be interesting for static-link as well. Any system library we build from source will require to be available in the IOC image, even with STATIC_BUILD=YES.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we discussed in person, I moved it to base.

@henriquesimoes henriquesimoes marked this pull request as draft November 8, 2023 19:25
@henriquesimoes henriquesimoes marked this pull request as ready for review November 22, 2023 13:53
Comment on lines 104 to 105
git clone https://github.com/cnpem/ssc-pimega
git -C ssc-pimega checkout $LIBSSCPIMEGA_VERSION
make -C ssc-pimega/c install
Copy link
Member

Choose a reason for hiding this comment

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

It is possible to download the tarball for a specific git hash, and IMO we should do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like the idea. Would you rather support hash in download_github_module or use a directly link for this case in particular?

Supporting only tags seems better to force us to use only released code. On the other hand, it makes it harder to handle such cases in which the project does not use tags as a way to release.

Copy link
Member

Choose a reason for hiding this comment

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

Supporting only tags seems better to force us to use only released code. On the other hand, it makes it harder to handle such cases in which the project does not use tags as a way to release.

That's a debate I have seen a lot of times, lol

Extending the function to support hashes would be best if we know we are going to depend on it. Who's to say we are never going to need bug fixes for which a patch isn't feasible? I think I'd go for it.

CHANGES.md Outdated Show resolved Hide resolved
This ensures the local library used by the compiler during the base
image build is the same during dynamic linkage, without regarding in the
configuration done in the IOC level through RUNTIME_TAR_PACKAGES. Note
this is not applicable to libraries installed via BUILD_TAR_PACKAGES.

This also allows to build libraries from source in the base image and
use their compiled binaries in the IOC image.

This assumes all libraries in `/usr/local/lib` are to be copied to the
final image. A cleaning right after the build should be performed to
avoid copying unnecessary artifacts.
`download_github_module` no longer allows only tags for download from
GitHub, but also versions specified by branches or commit hashes. This
makes it possible to download an unreleased (untagged) version of a
module, which applies (a series of) useful bug fixes and patches for our
deploy. Modules that do not use tags for versioning can also be
downloaded with this function now.

When specifying a version through its hash, the complete hash string (41
characters) must be provided due to the default name of directory inside
the archive provided by GitHub.
This plugin computes the geometric restoration for PiMega detectors
based on the LNLS ssc-pimega library, and allows Pimega IOC to provide
coherent data to users.

Even though ADPimega is not yet supported, this plugin can still be used
with ADSimDetector for debugging purposes. Furthermore, it makes it
possible to build ADPimega (and its IOC) out of epics-in-docker, but
still using NDSSCPimega from base.

ssc-pimega repository does not have tag for version 0.8.13. Thus, commit
hash is used instead.
@ericonr ericonr merged commit 185c6b4 into main Nov 29, 2023
@ericonr ericonr deleted the ssc-pimega branch November 29, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants