epics-modules / motor

APS BCDA synApps module: motor
https://epics-modules.github.io/motor/
20 stars 47 forks source link

motorOms's definition of SUPPORT breaks travis builds #158

Closed kmpeters closed 4 years ago

kmpeters commented 4 years ago

motorOms's definition of SUPPORT breaks travis builds:

https://travis-ci.org/github/epics-modules/motor/jobs/664040191#L788-L812

Despite changes to motorOms to allow the modules/RELEASE.linux-x86_64.local file to override SUPPORT:

https://github.com/epics-motor/motorOms/commit/7dd9732e9fb2ae5b7050f12adac71647f7b72e80

Edit: corrected line numbers in the travis log

kmpeters commented 4 years ago

Is it possible that modules/RELEASE.linux-x86_64.local doesn't exist when motorOms is being built? I can't imagine how that could be the case.

kmpeters commented 4 years ago

These lines get printed when I do a make in the modules directory:

Creating RELEASE.linux-x86_64.local, MOTOR = /net/s100dserv/xorApps/epics/motor-devel/support/motor-git
Creating RELEASE.linux-x86_64.local, ASYN = /net/s100dserv/xorApps/epics/motor-devel/support/asyn
Creating RELEASE.linux-x86_64.local, SNCSEQ = /net/s100dserv/xorApps/epics/motor-devel/support/seq-2-2-4
Creating RELEASE.linux-x86_64.local, BUSY = /net/s100dserv/xorApps/epics/motor-devel/support/busy
Creating RELEASE.linux-x86_64.local, IPAC = /net/s100dserv/xorApps/epics/motor-devel/support/ipac
Creating RELEASE.linux-x86_64.local, MODBUS = /net/s100dserv/xorApps/epics/motor-devel/support/modbus
Creating RELEASE.linux-x86_64.local, EPICS_BASE = /APSshare/epics/base-3.15.6

Those lines don't appear in the travis log: https://api.travis-ci.org/v3/job/664040191/log.txt

kmpeters commented 4 years ago

@anjohnson, do you have any ideas how travis is building submodules without the RELEASE.linux-x86_64.local file getting created in the modules directory?

anjohnson commented 4 years ago

@ralphlange might have an explanation but I suspect that Travis is actually suppressing some lines from the log output, without documenting that. I noticed last night that when building Base (and other projects like pvxs) it never logs the lines that get echoed when installing a C/C++ header (.h) file, but it does print "Installing ..." lines for other file-types. The same might be true for that "Creating RELEASE..." line.

ralphlange commented 4 years ago

Weird. @anjohnson Can you point me to such a place?

Note that the CI builds are run '-j2', i.e., log lines from parallel jobs are often interspersed and the error causing a bail-out may appear way above. Does the motor submodule setup work with parallel builds?

anjohnson commented 4 years ago

Go to any Base build job, such as 727.1 and in your browser search for "Installing". You'll find lines for installing .pm files but none for any .h files. The rules for those are identical. Similarly if you go to 731.1 and search for "Creating" there is only one hit, and not for the modules/RELEASE.linux-x86.local file.

MarkRivers commented 4 years ago

Does the motor submodule setup work with parallel builds?

Yes. I just did the following on a 20 core Linux machine and it built fine:

rm modules/RELEASE.linux-x86_64.local
make -sj clean uninstall
make -sj
tboegi commented 4 years ago

I will try to set up a Virtual machine and run the scripts by hand

tboegi commented 4 years ago

Another question Do we need to set up TOP somewhere ? Or how is this done, and I missed it ?

tboegi commented 4 years ago

https://travis-ci.org/github/tboegi/motor/jobs/664406328?utm_medium=notification&utm_source=email Then I miss the output of e.g. /.ci-local/travis/post-prepare.sh

However, if I click on the line, the the output is "unfolded", and I see a line like this: Here: SUPPORT = /home/travis/build/tboegi/motor/.. ASYN: SUPPORT = /home/travis/.cache

ralphlange commented 4 years ago

Yes, these are called folds.

ralphlange commented 4 years ago

Again: SUPPORT is a shortcut for manually setting up the RELEASE files. It is not needed for the Travis build, as all dependencies are added to RELEASE.local explicitly.

The build problem is that the definition for SUPPORT in the main motor module is different from those in the motor driver submodules. Adding a support definition to the generated file will not resolve the inconsistency between motor and its submodules.

ralphlange commented 4 years ago

Have you tried commenting out the SUPPORT definitions in the motor module and its submodules? I.e., do a regex-replace "^(SUPPORT=.*)$" with "#\1" in all files named RELEASE?

As the driver submodules' RELEASE files include the generated RELEASE.$(EPICS_HOST_ARCH).local in the modules directory, that file should set MOTOR to point to the parent and include the parent module's RELEASE (which gets overwritten by the ci-scripts to include the generated RELEASE.local pointing to the cache).

ralphlange commented 4 years ago

The generated RELEASE.$(EPICS_HOST_ARCH).local should not set SUPPORT. No one should set SUPPORT.

ralphlange commented 4 years ago

Note that any commenting of SUPPORT= would not catch a definition in the generated RELEASE.$(EPICS_HOST_ARCH).local, as it is being generated during the build, i.e. after your post-prepare.sh script is run. post-prepare.sh would have to patch the Makefile or whatever is generating RELEASE.$(EPICS_HOST_ARCH).local to not include the SUPPORT setting (or use a consistent one).

kmpeters commented 4 years ago

I modified post-prepare.sh so that it does NOT add SUPPORT to the RELEASE.local file and instead comments out SUPPORT in motor and motorOms's RELEASE files:

https://github.com/epics-modules/motor/blob/af59280dc797901fd7f2a733eda77d4f1c82e7b2/.ci-local/travis/post-prepare.sh#L15-L33

And now most of the builds are OK:

https://travis-ci.org/github/epics-modules/motor/builds/664873046

kmpeters commented 4 years ago

@MarkRivers, if I modify motor/configure/RELEASE so that SUPPORT is commented out by default, is the following change that defined SUPPORT in motorOms still necessary?

https://github.com/epics-motor/motorOms/commit/bef5c872c3f524e87d8e8c6f75b5ee1e11ffc5f7#diff-d5827f9ae7f09ee0bb6289a3a681a6ce

tboegi commented 4 years ago

I had a different approach, following @ralphlange suggestion to remove SUPPORT definitions completely for travis: https://travis-ci.com/github/tboegi/motor?utm_medium=notification&utm_source=email

ralphlange commented 4 years ago

What does motorOms need SUPPORT for? As long as there's only a single definition of SUPPORT (even the fake one in motor/config/RELEASE), the setup cannot be inconsistent.

The post-prepare.sh should not add SUPPORT to the ci-script generated RELEASE.local - that's definitely a good step.

ralphlange commented 4 years ago

Ah, I see, for the OPI conversion:

-include $(SUPPORT)/configure/RULES_OPI

That's bad, as it makes assumptions about the organization of source trees, and treats SUPPORT like a module (which it isn't). These RULES should be moved into a real module (ASYN? A module of its own?), and used via a regular module variable.

Note that the EPICS build system has a feature to properly install RULES snippets from inside a module that will automatically be included in all applications that depend on the module via their configure/RELEASE. That would be the proper thing to do.

MarkRivers commented 4 years ago

These RULES should be moved into a real module (ASYN? A module of its own?), and used via a regular module variable.

All modules that want to support autoconversion would then need to include that new module. I don't think we want to introduce unneeded dependencies on ASYN, so it would have to be a new module.

Note that the EPICS build system has a feature to properly install RULES snippets from inside a module that will automatically be included in all applications that depend on the module via their configure/RELEASE. That would be the proper thing to do.

What version of base does that feature start with? The current mechanism works with any version of base.

ralphlange commented 4 years ago

Here's is an example, where a Device Support module installs its CONFIG and RULES so that downstream applications automatically inherit the configuration and the rules necessary to use that support. https://github.com/ralphlange/opcua/tree/master/devOpcuaSup/UaSdk

Available in 3.15.2 and up.

ralphlange commented 4 years ago

The ...@ expansion of the CONFIG allows to "freeze" the relevant parts of the Device Support module build configuration into the installed CONFIG snippet, so that downstream modules inherit the proper configuration for this installation of the support module and don't have to manually make sure they use the same settings.

kmpeters commented 4 years ago

I had a different approach, following @ralphlange suggestion to remove SUPPORT definitions completely for travis: https://travis-ci.com/github/tboegi/motor?utm_medium=notification&utm_source=email

@tboegi, aren't the lines being appended to the configure/RELEASE in the following commit redundant? https://github.com/tboegi/motor/commit/6d93163797381993ae114d7af8678477ea748d60

I see those lines here: https://github.com/epics-modules/motor/blob/af59280dc797901fd7f2a733eda77d4f1c82e7b2/configure/RELEASE#L45-L49

I think the best solution will ultimately involve commenting SUPPORT out of RELEASE files and removing most of the lines from post-prepare.sh that perform the ugly hacks. I intend to leave post-prepare.sh there in case we need it in the future.

kmpeters commented 4 years ago

Here's is an example, where a Device Support module installs its CONFIG and RULES so that downstream applications automatically inherit the configuration and the rules necessary to use that support. https://github.com/ralphlange/opcua/tree/master/devOpcuaSup/UaSdk

Available in 3.15.2 and up.

What should one do if they still need to support 3.14?

ralphlange commented 4 years ago

Good question. next question.

As @MarkRivers wrote: Put the CONFIG and RULES into a separate module, and have downstream modules that want to use autoconversion refer to the module in configure/RELEASE and include the rules using the module tag.

-include $(OPICONV)/configure/RULES_OPI

or whatever name you choose.

kmpeters commented 4 years ago

I'm closing this issue since there is a workaround for the travis builds.

The autoconversion rule discussion is a good one to continue on tech-talk, if there is more to add to it.