Update install process to fix class ucode link directives #255

brucerennie · 2021-10-08T05:55:59Z

Changes made to the install process for make
1). addition of the compilation and execution of the program fixalllinks
after the installation of the Unicon system into the current system
2). addition of additional class directories that should be transferred on install
3). update the call of fixalllinks to ./fixalllinks (caused problem with debian install process)

Signed-off-by: Bruce Rennie [email protected]

Jafaral · 2021-10-08T13:22:19Z

@brucerennie DESTDIR can be passed in as an environment variable. It is used by the Debian package build system That is why your PR fails that test now, and the error shows as:

dh_auto_install: error: make -j1 install DESTDIR=/home/runner/work/unicon/unicondist/unicon_13.3\~prerelease/debian/unicon AM_UPDATE_INFO_DIR=no returned exit code 2
make: *** [debian/rules:22: binary] Error 25
dpkg-buildpackage: error: fakeroot debian/rules binary subprocess returned exit status 2
debuild: fatal error at line 1182:
dpkg-buildpackage -us -uc -ui failed
make: *** [Makefile:375: debin] Error 29
Error: Process completed with exit code 2.

Changes made to the install process for make 1). addition of the compilation and execution of the program fixalllinks after the installation of the Unicon system into the current system 2). addition of additional class directories that should be transferred on install Signed-off-by: Bruce Rennie <[email protected]>

It appears that the Debian install process requires ./fixalllinks instead of fixalllinks without the path specifier. Signed-off-by: Bruce Rennie <[email protected]>

Jafaral

Thank @brucerennie , I made a few suggestions, pleas let me know what you think.

Jafaral · 2021-10-25T03:37:04Z

Makefile

@@ -234,7 +244,7 @@ Tbins=unicon icont iconx iconc unicont uniconx uniconc udb uprof unidep unidoc \
 	ui ivib patchstr iyacc rt.a rt.h

 Tdirs=$(DESTDIR)$(ULB) $(DESTDIR)$(UIPL) $(DESTDIR)$(UPLUGINS)
-Udirs=lib 3d gui unidoc unidep xml parser
+Udirs=lib 3d gui ide unidoc unidep xml parser udb udb/dta udb/lib unicon


Why are we installing these extra directories? Are they being used as libraries similar to the existing dirs ?

The extra directories contain useful ucode files. In addition, they also have updateable link directives. I added them for completeness. If you do not want to include them that is your prerogative.

The reason those are not included as the other ones is that they don't contain library files intended to be used by end users. ide, unicon, udb, etc, are meant to be used in their binary forms. They are also not on the IPATH/LPATH like the other ones, and that is why they are not included by default.

Jafaral · 2021-10-25T03:42:54Z

fixalllinks.icn

@@ -0,0 +1,130 @@
+############################################################################
+#
+#	File:     fixalllinks.icn


let us give the a more descriptive name in this context, maybe fixucodelinks.icn ?

Also, let us move the file down to a subdirectory. I suggest to uni/progs since we already have utilities there such as idxGen.icn that we use with docs.

Any required name changes or location placement changes that you require are a "go" as far as I am concerned. Let me know exactly want you want here and I'll make the appropriate changes.

Thanks @brucerennie

Jafaral · 2021-10-25T03:44:49Z

fixalllinks.icn

+#
+#        fixalllinks install.dir
+#
+#       Within the Makefile, the install.dir is $(ULB)


The install location for "unicon library files" is $(ULB)

Not a problem, I will update the comments shortly.

Jafaral · 2021-10-25T03:57:29Z

fixalllinks.icn

+            fname,
+            bname,
+            line,
+            subdirectories := ["3d", "gui", "ide", "lib", "parser", "udb",


It is better if these subdirectories re passed in from the command line. Actually, we don't even have to do them all it was, but we could. Since the Makefile already does the follopwing:

@for d in $(Udirs); do \ echo "Installing uni/$$d to $(DESTDIR)$(ULB)/$$d ..."; \ $(INST) -m 644 uni/$$d/*.* $(DESTDIR)$(ULB)/$$d; \ done

we can just add a call to fix links as we go one directory at a time.

The reason for this is that we don't have change the file itself if we add/remove new subdirectory or if installing some of them became conditionals. Also, by generalizing it, a user can use it to patch ucode files for any reason when they are moved around.

I'll check first that we can run it in this position and make sure that the code doesn't break.

It is better if these subdirectories re passed in from the command line. Actually, we don't even have to do them all it was, but we could. Since the Makefile already does the follopwing:

@for d in $(Udirs); do \ echo "Installing uni/$$d to $(DESTDIR)$(ULB)/$$d ..."; \ $(INST) -m 644 uni/$$d/*.* $(DESTDIR)$(ULB)/$$d; \ done

we can just add a call to fix links as we go one directory at a time.

The reason for this is that we don't have change the file itself if we add/remove new subdirectory or if installing some of them became conditionals. Also, by generalizing it, a user can use it to patch ucode files for any reason when they are moved around.

If we change to command line parameters, Bruce will have to change the present treatment of the log file. Currently, it is opened with "w" (which will erase the previous log file). We will need a way of starting again at the beginning of a fresh install.

Don-Ward · 2021-10-26T19:55:30Z

fixalllinks.icn

+                    if exists(destdir || "/" || sd || "/" || infilename || ".old") then {
+                        remove(destdir || "/" || sd || "/" || infilename || ".old")
+                    }
+                    rename(destdir || "/" || sd || "/" || infilename, destdir || "/" || sd || "/" || infilename || ".old")


Rename can fail. Is it worth checking for that?

Don-Ward · 2021-10-26T20:06:19Z

fixalllinks.icn

+        #
+        every sd := !subdirectories do {
+            writes(logfile, "sub-directory:")
+            destdirfs := open(write(logfile, destdir || "/" || sd), "r")


I wonder if it's worth computing prefix := destdir || "/" || sd || "/" and then replacing the multiple instances of destdir || "/" || sd // "/" || ... with prefix || ...

It might not make all that much difference to the run time, but it might make the code slightly easier to read.

No worries if you disagree.

Don-Ward

Looks OK. I've made a couple of very minor comments but there are no show-stoppers that I can see.

Jafaral · 2021-12-13T04:25:30Z

Hi @brucerennie , any updates on this PR ?

brucerennie · 2021-12-13T06:16:31Z

I have converted this pull request to a draft as I have been unable to get back to it due to various other matters.

I will look at this again in the new year and make all appropriate updates per the various suggestions that have been made.

brucerennie requested a review from Jafaral October 8, 2021 06:25

brucerennie force-pushed the installchanges branch from d9a6012 to e328749 Compare October 8, 2021 10:32

brucerennie force-pushed the installchanges branch 2 times, most recently from 5a39cc9 to 77af74a Compare October 8, 2021 19:04

brucerennie force-pushed the installchanges branch from 77af74a to a74dccc Compare October 8, 2021 19:19

Adjust Makefile to change execution path of fixalllinks

8f35b4a

It appears that the Debian install process requires ./fixalllinks instead of fixalllinks without the path specifier. Signed-off-by: Bruce Rennie <[email protected]>

Jafaral requested changes Oct 25, 2021

View reviewed changes

Jafaral requested a review from Don-Ward October 25, 2021 03:58

Don-Ward reviewed Oct 26, 2021

View reviewed changes

brucerennie marked this pull request as draft December 13, 2021 06:14

brucerennie added 2 commits July 25, 2022 20:52

Merge branch 'uniconproject:master' into installchanges

6e5ccc0

Merge branch 'uniconproject:master' into installchanges

838442d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update install process to fix class ucode link directives #255

Update install process to fix class ucode link directives #255

brucerennie commented Oct 8, 2021 •

edited

Loading

Jafaral commented Oct 8, 2021

Jafaral left a comment

Jafaral Oct 25, 2021

brucerennie Oct 27, 2021

Jafaral Oct 28, 2021

Jafaral Oct 25, 2021

brucerennie Oct 27, 2021

Jafaral Oct 28, 2021

Jafaral Oct 25, 2021

brucerennie Oct 27, 2021

Jafaral Oct 25, 2021

brucerennie Oct 25, 2021

Don-Ward Oct 26, 2021

Don-Ward Oct 26, 2021

Don-Ward Oct 26, 2021

Don-Ward left a comment

Jafaral commented Dec 13, 2021

brucerennie commented Dec 13, 2021

Update install process to fix class ucode link directives #255

Are you sure you want to change the base?

Update install process to fix class ucode link directives #255

Conversation

brucerennie commented Oct 8, 2021 • edited Loading

Jafaral commented Oct 8, 2021

Jafaral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-Ward left a comment

Choose a reason for hiding this comment

Jafaral commented Dec 13, 2021

brucerennie commented Dec 13, 2021

brucerennie commented Oct 8, 2021 •

edited

Loading