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

doit 0.28 release - breaking Nikola (or not) #1655

Merged
merged 11 commits into from Apr 26, 2015
Merged

doit 0.28 release - breaking Nikola (or not) #1655

merged 11 commits into from Apr 26, 2015

Conversation

Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 11, 2015

I am planning to release doit 0.28 next week.

A) it would be good if you guys make a release with the doit pinned version before I release 0.28, this way at least new installation would get the correct doit version. is it possible?

B) I managed to get a patch[1] in doit that seems to be good enough to maintain backwards compatibility (at least for Nikola). But I didnt test much... note this is not on master branch.

Another option would be to make doit detect when Nikola is using the old API and give an error message like: "doit version higher than supported version. Please downgrade to doit 0.27 with: pip install doit=0.27"

IMO the second option is safer. If you guys prefer the patch I am also ok with, but please test it a bit.
So please let me what you guys think...

[1] pydoit/doit@8666f73

@Kwpolska Kwpolska added this to the v7.3.2 milestone Apr 10, 2015
@Kwpolska Kwpolska self-assigned this Apr 10, 2015
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 10, 2015

I’ll try to look into it. The best thing to do would be to make Nikola compatible with doit 0.28 and release quickly. There isn’t much holding us back from a release.

@schettino72
Copy link
Member Author

@schettino72 schettino72 commented Apr 10, 2015

The best thing to do would be to make Nikola compatible with doit 0.28 and release quickly.

Even with that I guess we should worry about old Nikola installations..
Also a change to use doit 0.28 IMO should bump Nikola version to 7.4.0.

The main change that affects Nikola is that:

  • doit 0.27 DoitMain.get_commands() return Command instances
  • doit 0.28 DoitMain.get_cmds() returns Command classes

Now you guys won't even need to sub-class get_cmds()... another change is that Command.__init__ must take a **kwargs.

I am planning to prepare a patch for Nikola to use doit 0.28 tomorrow.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 11, 2015

Now you guys won't even need to sub-class get_cmds()...

And how would we tell doit about our custom commands?


Currently, Nikola is broken badly. You see, we instantiate all our plugins — and doit does not anymore. We can’t change that, yapsy always does that. The following problems surfaced:

  • the isinstance used to check for nikola help in __main__.py was broken — rewritten as is not Help
  • nikola help used cmd.name which was None for uninitialized doit commands — copy-pasted from new doit implementation
  • running a command tries to instantiate it — rewritten as a __call__ hack

Please review the patch in fb19e2d.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 11, 2015

It might be more helpful to review the entire branch instead: https://github.com/getnikola/nikola/compare/doit-0.28.0

@schettino72
Copy link
Member Author

@schettino72 schettino72 commented Apr 11, 2015

nikola/utils.py:Commands is broken too. This is not used by Nikola itself. Is it for plugins?

@@ -93,6 +93,10 @@ def __init__(self, *args, **kwargs):
BasePlugin.__init__(self, *args, **kwargs)
DoitCommand.__init__(self)

def __call__(self, config=None, **kwargs):
Copy link
Member

@schettino72 schettino72 Apr 11, 2015

Choose a reason for hiding this comment

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

amazing hack :)

Looking at yapsy you could sub-class PluginManager.instanciateElement() to avoid this.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 11, 2015

It’s a friendly way to run things via nikola console. I’ll look into it later.

@schettino72
Copy link
Member Author

@schettino72 schettino72 commented Apr 11, 2015

Now you guys won't even need to sub-class get_cmds()...

And how would we tell doit about our custom commands?

Sorry. Ignore that. You would need to pass the entry point instead of the object. Doesnt make sense for Nikola because it already load it through plugins.

@schettino72
Copy link
Member Author

@schettino72 schettino72 commented Apr 15, 2015

BUMP. Any expected date for a release? no pressure 😁

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 22, 2015

@schettino72 So, doit==0.28.0 is out. I don’t feel confident releasing anything right now (neither master pinned to 0.27.0 nor the new branch). As such, I made an emergency v7.3.1.1 release that only changes the doit version and nothing else.

@schettino72
Copy link
Member Author

@schettino72 schettino72 commented Apr 22, 2015

Yes. it is out. I added a notice if any program tries to use the old API (I tested it with Nikola). So at least the user gets a nice error message.

pydoit/doit@0606570

Kwpolska added 6 commits Apr 24, 2015
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 24, 2015

@schettino72 How can I get instances of doit-provided commands? The site.commands proxy and nikola check both require them: site.commands needs to run get_options() (instance-bound), nikola check needs to talk to nikola list with the current context.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 24, 2015

And for the record, current things that need to change:

  • nikola.utils.Commands needs to iterate over .get_cmds() and use v.get_options()
  • nikola.plugins.command.check needs to access list differently

Kwpolska added 2 commits Apr 24, 2015
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 24, 2015

@schettino72 Is this really the way to go?

self._doitargs['cmds']['list'](config=self.config, **self._doitargs)

(self._doitargs = **kwargs in the __call__ hack)

If yes, Commands would need to be generated by all plugins that need it (probably only nikola console) with a crazy incantation like this…

@schettino72
Copy link
Member Author

@schettino72 schettino72 commented Apr 24, 2015

@Kwpolska look at the doit help command for an example.

  1. get the list of command class in the constructor, (and also save the other kwargs parameters)
  1. Just create commands instances as you wish :)

So, if you save the parameters with some nice names, it doesnt look so much like a crazy incantation :D

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 25, 2015

I’m more of a singleton fan, but okay. I’m leaving the spaghetti in check and rewriting the Command thing.

Everything should be fine now.

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 25, 2015

Needs review. @schettino72 @ralsina?

self._main = main
self._config = config
self._doitargs = doitargs
for k, v in self._main.get_cmds().items():
Copy link
Member

@schettino72 schettino72 Apr 25, 2015

Choose a reason for hiding this comment

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

You should use the cmds provided on doitargs, so please don't use DoitMain.get_cmds() .

Copy link
Member

@schettino72 schettino72 Apr 25, 2015

Choose a reason for hiding this comment

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

Note that cmds is a PluginDict [1], not a plain dict. You should get its items using cmds.todict(). Or if you need just one value, cmds.get_plugin().

Plugins are lazy-loaded... for core commands plugins are not used so you get the command class without problems. But for plugins the dict value will only contain the entry point.

[1] https://github.com/pydoit/doit/blob/master/doit/plugin.py#L53

Copy link
Member Author

@Kwpolska Kwpolska Apr 25, 2015

Choose a reason for hiding this comment

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

fixed in b9f081e

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 26, 2015

Merging — I hope nothing will break.

Kwpolska added a commit that referenced this issue Apr 26, 2015
doit 0.28 release - breaking Nikola (or not)
@Kwpolska Kwpolska merged commit 0b93441 into master Apr 26, 2015
2 of 3 checks passed
@Kwpolska Kwpolska deleted the doit-0.28.0 branch Apr 26, 2015
felixfontein
Copy link
Contributor

@felixfontein felixfontein commented on a73ed63 May 17, 2015

Choose a reason for hiding this comment

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

This creates a problem with Python 3: 'a' == b'a' under Python 2, while 'a' != b'a' under Python 3. Now under Python 3, repr(arg[:7]) is '--conf=' for me, whence the two strings are never equal and I get ERROR: Nikola: Unknown command --conf=xxx.

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented on a73ed63 May 17, 2015

Choose a reason for hiding this comment

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

(Replacing b'--conf=' with '--conf=' helps, or replacing arg[:7] by arg[:7].encode('utf-8').)

Kwpolska
Copy link
Member Author

@Kwpolska Kwpolska commented on a73ed63 May 17, 2015

Choose a reason for hiding this comment

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

would you mind figuring a solution that works with both Pythons and making a PR?

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented on a73ed63 May 17, 2015

Choose a reason for hiding this comment

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

What was the motivation to change '--conf=' to b'--conf=' in the first place? I think before this change it was working fine.

Kwpolska
Copy link
Member Author

@Kwpolska Kwpolska commented on a73ed63 May 17, 2015

Choose a reason for hiding this comment

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

it wasn’t, we were getting “cannot coerce to Unicode” errors if there were Unicode arguments (which can happen with nikola new_post -t, for instance)

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented on a73ed63 May 17, 2015

Choose a reason for hiding this comment

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

Then adding .encode('utf-8') to arg is probably the best fix. From what I understand, this works fine under Python 2 and Python 3, and shouldn't exhibit the "cannot coerce to Unicode" errors. What do you think?

This also affects some more settings parsing in main.py, I think.

Kwpolska
Copy link
Member Author

@Kwpolska Kwpolska commented on a73ed63 May 17, 2015

Choose a reason for hiding this comment

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

I don’t have time to look into this further, I’ll try to come up with a solution tomorrow. And yes, we would need to fix every argument check in the file.

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented on a73ed63 May 20, 2015

Choose a reason for hiding this comment

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

Did you had a chance to look at it?

Kwpolska
Copy link
Member Author

@Kwpolska Kwpolska commented on a73ed63 May 20, 2015

Choose a reason for hiding this comment

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

didn’t have time; see #1738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants