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

Restructured folder layout #318

Merged
merged 1 commit into from
Nov 10, 2016
Merged

Restructured folder layout #318

merged 1 commit into from
Nov 10, 2016

Conversation

kaikreuzer
Copy link
Member

@kaikreuzer kaikreuzer commented Nov 6, 2016

  • moved runtime/karaf/etc folder to userdata/etc
  • removed runtime/etc, only kept runtime/services.cfg (moved from runtime/etc/services.cfg)
  • moved runtime/karaf to runtime
  • moved userdata/lock to userdata/tmp/lock
  • removed userdata/deploy
  • removed userdata/kar

Fixes #315
Fixes #225

Signed-off-by: Kai Kreuzer kai@openhab.org

@kaikreuzer
Copy link
Member Author

This PR introduces some changes in the directory structure that might have impact on other parts.

@theoweiss Could you please check if anything needs to be adapted for the debian packaging? Please note that with that change the start.sh script in the root is adapted, so this needs to be taken into account.

@ThomDietrich Can you assess the implications on the documentation? I didn't see too much that would need to be adapted.

@cniweb Please check that Docker and Synology images correctly handle this change.

- moved etc folder to userdata
- moved runtime/karaf to runtime
- moved userdata/lock to userdata/tmp/lock
- removed userdata/deploy
- removed runtime/etc, only kept runtime/services.cfg

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@theoweiss
Copy link
Member

@kaikreuzer I'll check it.

@thopiekar
Copy link
Member

Thank you @cniweb for the mention and keeping me up to date..

Are there other changes except of the runtime/karaf/bin path which has changed?
Are our directories, which we are purging after an update, still save to remove?
See: https://github.com/openhab/openhab-qnap-qpkg/blob/master/shared/openHAB.sh#L141

Regards and thank you for your work on that, @kaikreuzer 👍

@ThomDietrich
Copy link
Member

Regarding docs:

The first two are quick changes I can do after this PR is merged.

One easy change is also needed in openHABian: openhab/openhabian#34

@cniweb
Copy link
Member

cniweb commented Nov 6, 2016

@kaikreuzer
Copy link
Member Author

Thank you @cniweb for the mention and keeping me up to date..

Sorry @thopiekar for having missed you - I somehow didn't remember that your QNAP package also does OH2 :-) Will keep you in the loop in future as well :-)

All, thanks for your prompt feedback! So in general, I take your responses as an approval of this change.

For the planning, I would suggest to merge this on Thursday, if nobody vetoes.

@thopiekar
Copy link
Member

@kaikreuzer You are not alone here after all and at the end I was mentioned here, too. So at the end no problem at all.

Just one question left: Do we have to care about keeping the current settings save on updates/upgrades? Eg. moving the settings from the old to the new locations?

@kaikreuzer
Copy link
Member Author

@thopiekar As so far the settings always got lost with every update, I don't think we have to take care of that - not sure though, how you have handled the upgrade procedure so far? Did you keep runtime/karaf/etc folder in place?

@thopiekar
Copy link
Member

@kaikreuzer So far I followed the instructions by a user from the forums and just replaced the runtime and removed the userdata/cache and userdata/tmp, as you can see here: https://github.com/openhab/openhab-qnap-qpkg/blob/master/shared/openHAB.sh#L141

Until now I didn't know that I also had to care about runtime/karaf/etc 🙄

@kaikreuzer
Copy link
Member Author

Until now I didn't know that I also had to care about runtime/karaf/etc

Nobody knew and soon nobody has to know :-)

But good, your procedure is then very much in line with what I suggest in #299.

@thopiekar
Copy link
Member

#299 sounds good. Hope we'll find more problems there.
I already had strange effects e.g. when using the Zwave binding, where reinstalling was the only and last solution. However, maybe they will be fixed with this PR 💃

@kgoderis
Copy link
Contributor

kgoderis commented Nov 7, 2016

I think we should also move org.apache.karaf.shell.cfg, specifically for the sshPort and sshHost parameters that are typically modified by users

Moreover, as a Mac OS user, I also have to save and restore any openhab-wrapper_._ related files in order to keep things running. That is not only runtime/karaf/etc/openHAB-wrapper.conf and siblings, but also runtime/karaf/lib/wrapper/*

@kaikreuzer
Copy link
Member Author

@kgoderis See the very first line in this issue. The whole etc folder is moved.
Regarding the wrapper: This anyhow is currently broken (at least according to my tests). Feel free to come up with changes to https://github.com/openhab/openhab-core/blob/8a7f5561b1652d1c080d2a7fdc823c8d4bf93b31/bundles/org.openhab.core.karaf/src/main/java/org/openhab/core/karaf/internal/command/InstallServiceCommand.java to make sure that nothing relevant is written in the runtime folder.

@thopiekar
Copy link
Member

@kaikreuzer I'm preparing a PR at the moment for QPKG.
Is there a need to keep runtime/services.cfg during update? My current procedure would remove this file.

@ThomDietrich
Copy link
Member

ThomDietrich commented Nov 8, 2016

Without being able to give a clear answer on the question, here's a more general idea:
I would expect a file named runtime/services.cfg to be of some importance and to contain settings a user/routine should not delete... If this is not the case the file might be named incorrectly... jm2c...

@kaikreuzer
Copy link
Member Author

runtime/services.cfg contains the basis configuration settings of the runtime, it must not be changed by the user - therefore it does not have to be kept on a system update (the new runtime actually expects to find the current version of the file). That's why it is in the runtime folder, which is meant to be fully replaced as mentioned here.

cniweb added a commit to openhab/openhab-syno-spk that referenced this pull request Nov 8, 2016
Fix for: #37

Needed for: openhab/openhab-distro#318

Signed-off-by: Christian Häussler <c-n-i@web.de> (github: cniweb)
cniweb added a commit to openhab/openhab-docker that referenced this pull request Nov 8, 2016
Fix for #36

Needed for: openhab/openhab-distro#318

Signed-off-by: Christian Häussler <c-n-i@web.de> (github: cniweb)
@BClark09
Copy link
Member

The items/sitemaps won't refresh unless the path is set correctly. The logs can stay where they're being set and won't break the package, but I assumed the apt packages still wanted logs in /var/log like other services instead of /var/lib.

@ThomDietrich
Copy link
Member

Judging by the comments in the forum I think the mv (or as a cp) should have been part of your solution...

@BClark09
Copy link
Member

I'll edit anywhere I've mentioned it. Meanwhile, I'm working on a PR to fix the issue for the deb package.

@ThomDietrich
Copy link
Member

Good idea! @theoweiss and @thopiekar might be interested to know.

@BClark09
Copy link
Member

I believe I've made the appropriate necessary changes at: https://github.com/BClark09/openhab-distro/tree/deb-fix

But I'm currently testing it before I make the PR.

@ThomDietrich
Copy link
Member

Looks good but I'm missing special upgrade steps. As I see it this will deliberately damage every upgrading users setup!?
I think the right place to put the needed rearrangement of files and folders as per the first posting is here: https://github.com/BClark09/openhab-distro/blob/master/distributions/distribution-resources/src/main/resources/deb/control-runtime/preinst

@BClark09
Copy link
Member

this will deliberately damage every upgrading users setup!?

That sounds scary, in what way?

@ThomDietrich
Copy link
Member

I'm sorry if I'm wrong! e.g. https://community.openhab.org/t/new-folder-structure/16526/13
I believe this happened because the host key file is not carried on

@BClark09
Copy link
Member

BClark09 commented Nov 11, 2016

This happens because it's moved, it would have been replaced each time before this PR anyway right?

ssh-keygen -R openhab@localhost might be a step that's required only when upgrading pre to post this PR. But I wouldn't know how I should check for that?

Since I'm currently running on the build Debian package of my branch and I haven't found any problems, I think I should make a PR and let @theoweiss give the all clear?

@ThomDietrich
Copy link
Member

ThomDietrich commented Nov 11, 2016

Okay I'm not sure either. I think priority is to get a fix for the more eminent problem out there.
Create the PR and either Theo or Kai can decide if it's worth to wait.

Still I think doing what Kai mentioned as changes while the upgrade would definitely not hurt...

if folder exists "runtime/karaf/etc":
mv /usr/share/openhab2/runtime/karaf/etc/* /var/lib/openhab2/etc/
rm -r /var/lib/openhab2/{kar,lock,deploy}
...

@sihui62
Copy link

sihui62 commented Nov 11, 2016

it would have been replaced each time before this PR anyway right?

Just to mention: I'm using the online openHAB2 version (manually installed, no apt) and I had to use the command ssh-keygen -f "/home/pi/.ssh/known_hosts" -R [localhost]:8101 on EVERY update of openHAB2. So from my experience it has nothing to do with this PR. (but I'm only a user, not a programmer ...)

@ThomDietrich
Copy link
Member

The error message says "REMOTE HOST IDENTIFICATION HAS CHANGED" and "It is also possible that a host key has just been changed".
I see this message from time to time when I reinstall a server or a RPi and the key changes. In openHAB the key for the karaf console is runtime/karaf/etc/host.key (more or less). So yes the key can simply be deleted from your known_hosts, you could also just carry it with while upgrading ;) There are other files in /runtume/karaf/etc users might have modified and worth to be copied... jmho

@BClark09
Copy link
Member

BClark09 commented Nov 11, 2016

Fair enough, I agree, do you think the following in preinst would be sufficient?

#...
OLD_ETC=/var/lib/openhab2/etc/
NEW_ETC=/usr/share/openhab2/runtime/karaf/etc/
#...
moveOldDir(){
  [ ! -d ${NEW_ETC} ] && mkdir -p ${NEW_ETC}
  [ -d ${OLD_ETC} ] && mv ${OLD_ETC}* ${NEW_ETC} && rm -rf ${OLD_ETC}
  rm -rf /var/lib/openhab2/{kar,lock,deploy}
  true
}
#...
  upgrade)
    moveOldDir
    ;;
#...

We should probably continue this conversation on the PR.

@thopiekar
Copy link
Member

thopiekar commented Nov 11, 2016

@BClark09 Are you sure that this script works like expected by the most people?
I personally see the problem that APT will fail to install as it will find unexpectedly the config files.
In my point of view I would move it into postinstall, just before the postinstall starts the service itself.

The docs say it should work like this without errors..

See: https://wiki.debian.org/DpkgConffileHandling - section: "Moving a conffile"

Btw. they argue the same:

If we moved the conffile in the preinst script, dpkg would unpack the new package and see that the conffile has changed and ask the user what to do about it. We want to avoid that, so we let dpkg unpack the package without conflict. We can now handle moving the user-changed conffile into place in the postinst script.

But I think it depends on which behavior we want here...

@ThomDietrich
Copy link
Member

I'm no expert here by all means.
I decided to go with preinst because I want these files to be handled by the installer. Just as it was before.

@thopiekar
Copy link
Member

@ThomDietrich: Ok. So if we want feedback from the user, then this should be correct 😉

@BClark09
Copy link
Member

BClark09 commented Nov 11, 2016

I think we should be okay for this instance because we're moving non-conf files to become conf files. The majority of the file contents will not have changed and if they have then of course we should prompt the user.

We definitely don't want to do this post-install because you'll be overwriting any necessary changes we make at a later date.

EDIT: The script fails the package if the folder exists but is empty, I'm omitting it for now.

@thopiekar
Copy link
Member

Ok, so there are no problems by openhab-distro itself? (Without looking at the problems we have in our different packages..)

@ThomDietrich
Copy link
Member

Aside the changes to the deb related files my system seems to work again. We will see...
@BClark09 I've corrected my script over in the PR comment. Maybe it's worth to be included after all...

ThomDietrich pushed a commit to openhab/openhab-docs that referenced this pull request Nov 12, 2016
CHanges related to openhab/openhab-distro#318


* Add HABmin and HABPanel UIs to Overview

Signed-off-by: Christian Häussler <c-n-i@web.de> (github: cniweb)

* Update working devices

openhab/openhab-syno-spk#29

Signed-off-by: Christian Häussler <c-n-i@web.de> (github: cniweb)

* Updating paths for new OH2 folder layout

Fix for #129

Needed for: openhab/openhab-distro#318

Signed-off-by: Christian Häussler <c-n-i@web.de> (github: cniweb)

* Updating paths for new OH2 folder layout

Fix for #129

Needed for: openhab/openhab-distro#318

Signed-off-by: Christian Häussler <c-n-i@web.de> (github: cniweb)

* Updating paths for new OH2 folder layout

Fix for #129

Needed for: openhab/openhab-distro#318

Signed-off-by: Christian Häussler <c-n-i@web.de> (github: cniweb)

* Updating paths for new OH2 folder layout

Fix for #129

Needed for: openhab/openhab-distro#318

Signed-off-by: Christian Häussler <c-n-i@web.de> (github: cniweb)

* Improve console interface text

* Correct path information, clarify details

* Update logging.md

* Remove redundant already commented content

* Add exlicit syntax to code fences

Signed-off-by: Thomas Dietrich <thomas.dietrich@tu-ilmenau.de>
@ThomDietrich
Copy link
Member

@kaikreuzer docs.openhab.org is up to speed.

@llamahunter
Copy link

What's the status of fixing the broken paths?

@ThomDietrich
Copy link
Member

The problem with broken paths is related only to deb based installation. Discussed in a new issue: #322

@ThomDietrich
Copy link
Member

@kaikreuzer Same goes for openHABian

@kgoderis
Copy link
Contributor

Regarding the wrapper: This anyhow is currently broken (at least according to my tests). Feel free to come up with changes to https://github.com/openhab/openhab-core/blob/8a7f5561b1652d1c080d2a7fdc823c8d4bf93b31/bundles/org.openhab.core.karaf/src/main/java/org/openhab/core/karaf/internal/command/InstallServiceCommand.java to make sure that nothing relevant is written in the runtime folder.

Not sure that this is even possible, as the call to wrapperService.install() seems to imply that there is little control as to which folders will be used. I am by no means a Karaf expert, so maybe @dvanherbergen you can have a look at this? [Secondly, I have no clue how to set up the IDE so that it uses Karaf for debugging purposes]

cniweb added a commit to openhab/openhab-syno-spk that referenced this pull request Nov 13, 2016
Fix for: #37

Needed for: openhab/openhab-distro#318

Signed-off-by: Christian Häussler <c-n-i@web.de> (github: cniweb)
@kaikreuzer kaikreuzer modified the milestone: 2.0.0.b5 Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants