Skip to content

Conversation

martinpitt
Copy link
Contributor

@martinpitt martinpitt commented Feb 3, 2017

This was done with:

  1. Manually renaming find -iname '*storage*'. Note that we use
    udisks2.spec instead of udisks.spec to get back to the previous
    package name in Fedora/RHEL.

  2. Automatic mass-renaming with
    git ls-tree -r --name-only HEAD|grep -v NEWS | xargs sed -ri
    's/storagedtestcase/udiskstestcase/g; s/Storaged([[:alnum:]]*)Test/Udisks\1Test/g;
    s/storaged.spec/udisks2.spec/g; s/storaged(_|\b)/udisks\1/g;
    s/Storaged/Udisks/g; s/storagectl/udisksctl/g; s/udisks-project/storaged-project/g'

    Note that we must not touch the checked STORAGED_* udev properties;
    they are already only a fallback for the UDISKS_* properties, and we
    should keep them for backwards compatibility with user-created udev
    rules for a while. We also keep "storaged-project" as GitHub owner
    as that's hard to change and not very visible in releases anyway.

  3. Remove the "is a fork.." message from README.md, and change
    top-level project name in NEWS.

  4. Review the remaining potential names and ensure they are unrelated:
    git grep -i storage| grep -Evi '(^NEWS|libstoragemgmt|STORAGED_|Rapid Storage|vsphere-50-storage|Apple Core Storage)'


As discussed last week. A make install works and looks plausible, src/tests/integration-test works, and make check succeeds after also applying the fix in PR #190 . The latter also fails its integration tests, and they are not visible to me; I figure this will need some corresponding changes in the test infrastructure and zatana.

@@ -48,9 +48,9 @@ def setup_vdevs():
for d in vdevs:
with open('/sys/block/%s/device/model' %
os.path.basename(d)) as model_file:
assert model_file.read().strip() == 'storaged_test_d'
assert model_file.read().strip() == 'udisks_test_d'
Copy link
Member

Choose a reason for hiding this comment

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

This should be renamed to "udisks_test_dis". (Model of the targetcli devices is first 15 letters of the disk name.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that might explain the odd test failure, thanks! Pushed.

@martinpitt
Copy link
Contributor Author

What the heck, the tests succeeded -- that is a bit of a (positive!) surprise, I would have expected some more fallout ☺

Copy link
Member

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

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

Looks good to me other than the few nitpicks.

We still need to do some packaging, Zanata (translations) and GitHub "magic" before merging this.

README.md Outdated
@@ -1,16 +1,13 @@
[![build status](https://phatina.fedorapeople.org/jenkins/storaged/build.svg)](https://phatina.fedorapeople.org/jenkins/storaged/build.log)
[![build status](https://phatina.fedorapeople.org/jenkins/udisks/build.svg)](https://phatina.fedorapeople.org/jenkins/udisks/build.log)
Copy link
Member

Choose a reason for hiding this comment

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

You can delete this. @vpodzime is working on a new way to publish test results.

@@ -335,7 +335,7 @@ systemctl try-restart udisks2
parted calls

* Wed Mar 16 2016 Peter Hatina <phatina@redhat.com> - 2.6.0-2
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't want to change the changelog.

po/da.po Outdated
@@ -42,7 +42,7 @@ msgid ""
"Mount/unmount filesystems defined in the fstab file with the x-udisks-auth "
"option"
msgstr ""
"Monter/afmonter filsystemer defineret i fstab-filen med tilvalget x-storaged-"
Copy link
Member

Choose a reason for hiding this comment

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

We probably should just skip all PO files. PO files are downloaded from Zanata, we should just update the POT file and download new PO files with next release or translation update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems better to me to actually do this. First of all, the current *.po files are wrong: They translate x-udisks-... to x-storaged-..., probably a victim of the earlier rename in the other direction. Second, if we can fix them mechanically I don't see the need of imposing manual labor on translators. And thirdly, as the msgid did not actually change, the POT file will not change either, so this wouldn't even do anything.

However, one instance of translating "Intel Rapid Storaged Technology" crept in, due to the typo (the 'd' should not be there). I'll fix that properly.

Copy link
Member

Choose a reason for hiding this comment

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

I've checked Zanata a there are some new translations (our PO files were downloaded in October) -- I'm not sure how Zanata handles conflicts when uploading new PO files and I really don't want to risk it. I now have admin rights for storaged project in Zanata, so I can fix it manually (it's just 1 string for 6 langugages so it should be easier).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll drop it from the patch then, and just keep the (unrelated) typo fix in de.po.

@@ -15,16 +15,16 @@

def find_daemon(projdir, system):
if not system:
if os.path.exists(os.path.join(projdir, 'src', 'storaged')):
Copy link
Member

Choose a reason for hiding this comment

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

You can just delete this "if". It was just for backwards compatibility when changing from storaged to udisks2 API.

elif os.path.exists(os.path.join(projdir, 'src', 'udisksd')):
daemon_bin = 'udisksd'
else:
print("Cannot find the daemon binary", file=sys.stderr)
sys.exit(1)
else:
if os.path.exists('/usr/libexec/storaged/storaged'):
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@@ -139,9 +139,9 @@ def setUpClass(self):
if daemon_bin == 'udisksd':
self.iface_prefix = 'org.freedesktop.UDisks2'
self.path_prefix = '/org/freedesktop/UDisks2'
elif daemon_bin == 'storaged':
Copy link
Member

Choose a reason for hiding this comment

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

Same here, you can just remove the storaged "if" here.

@vpodzime
Copy link
Collaborator

vpodzime commented Feb 6, 2017

Jenkins, test this, please.

@martinpitt
Copy link
Contributor Author

Force-pushed with addressing @vojtechtrefny 's review comments, thanks!

@martinpitt
Copy link
Contributor Author

@vojtechtrefny: Other than renaming the project on github, is there any other "github magic" to be done here?

@tsmetana
Copy link
Contributor

tsmetana commented Feb 7, 2017

I have no idea for a better place to write this: please don't forget about https://github.com/storaged-project/storaged-project.github.io (and storaged.org). They also deserve to be kept up-to-date.

@vojtechtrefny
Copy link
Member

@vojtechtrefny: Other than renaming the project on github, is there any other "github magic" to be done here?

I hope not, but I'm pessimist, so I always expect some problems. I'll probably create some new project/repo and try to rename it to see how everything works (especially if the old links still work etc.).

Copy link
Member

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

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

Found two more small things.

data/.gitignore Outdated
org.storaged.*.policy
org.udisks.*.conf
org.udisks.*.service
org.udisks.*.policy
*.pc
Copy link
Member

Choose a reason for hiding this comment

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

These should be org.freedesktop.*.

@@ -9,7 +9,7 @@
<releaseinfo>
For version &version; — the latest version of this
documentation can be found at <ulink role="online-location"
url="http://storaged.org/doc/udisks2-api/">http://storaged.org/doc/udisks2-api/</ulink>.
url="http://udisks.org/doc/udisks2-api/">http://udisks.org/doc/udisks2-api/</ulink>.
Copy link
Member

Choose a reason for hiding this comment

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

This should be http://storaged.org.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks. So I suppose I'll change https://udisks.freedesktop.org/docs/latest/ to be a redirect to storaged.org once we deploy this. From an access policy POV this is probably easier.

@vojtechtrefny
Copy link
Member

I tried to build udisks2 package in Copr and found few more problems in the SPEC file.

  • Name of the package has to be "udisks2", because we still have udisks package providing the "old" API.
  • We need to provide/obsolete all the storaged packages.
  • There was some old description about storaged replacing udisks.

Patch for this is here -- https://vtrefny.fedorapeople.org/patches/storaged-udisks-spec.patch -- and udisks succesfully builds with this in Copr -- https://copr.fedorainfracloud.org/coprs/vtrefny/test/build/509621/

@martinpitt
Copy link
Contributor Author

Thanks @vojtechtrefny! Force-pushed with rebasing against master, resolving the issues you found, and I added your spec file updates.

@martinpitt
Copy link
Contributor Author

Rebased to current master to resolve conflicts.

@vpodzime
Copy link
Collaborator

This needs one more rebase, sorry. But it looks good to me otherwise, so once rebased, we shall merge this ASAP. :)

martinpitt and others added 2 commits February 15, 2017 13:12
This was done with:

 1. Manually renaming `find -iname '*storage*'`. Note that we use
    udisks2.spec instead of udisks.spec to get back to the previous
    package name in Fedora/RHEL.

 2. Automatic mass-renaming with
    git ls-tree -r --name-only HEAD|grep -v NEWS | xargs sed -ri \
        's/storagedtestcase/udiskstestcase/g; s/Storaged([[:alnum:]]*)Test/Udisks\1Test/g;
         s/storaged.spec/udisks2.spec/g; s/storaged(_|\b)/udisks\1/g;
         s/Storaged/Udisks/g; s/storagectl/udisksctl/g; s/udisks-project/storaged-project/g'

    Note that we must not touch the checked STORAGED_* udev properties;
    they are already only a fallback for the UDISKS_* properties, and we
    should keep them for backwards compatibility with user-created udev
    rules for a while. We also keep "storaged-project" as GitHub owner
    as that's hard to change and not very visible in releases anyway.

 3. Remove the "is a fork.." message from README.md, and change
    top-level project name in NEWS.

 4. Review the remaining potential names and ensure they are unrelated:
    git grep -i storage| grep -Evi '(^NEWS|libstoragemgmt|STORAGED_|Rapid Storage|vsphere-50-storage|Apple Core Storage)'

 5. Fix the expected model file in src/tests/dbus-tests/run_tests.py
    (first 15 letters of the disk name).

 6. Fix the typo in the "Rapid Storaged Technology" in German
    translations.

 7. Various smaller reversions of the above seddery which are
    unnecessary.
@martinpitt
Copy link
Contributor Author

Rebased.

@vpodzime
Copy link
Collaborator

Rebased.

Thanks!

@vpodzime vpodzime merged commit d8ffd86 into storaged-project:master Feb 15, 2017
@martinpitt martinpitt deleted the the-grand-reunion branch February 15, 2017 14:12
@martinpitt
Copy link
Contributor Author

Whee, thanks! And I see you renamed the git project too, and the automatic redirect is working nicely.

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.

4 participants