Created attachment 1277962 [details] buildlog Description of problem: If pip package 'isort' is installed, engine build fails with message "ERROR: /home/jakub/src/ovirt-engine/packaging/setup/tests/ovirt_engine_setup/engine_common/test_database.py Imports are incorrectly sorted." Version-Release number of selected component (if applicable): 4.2 master, commit 3a600e0 How reproducible: 100% Steps to Reproduce: 1. sudo pip install isort 2. trigger engine compilation 3. Actual results: Error during build, see attachment Expected results: Engine is compiled regardless of isort installation Additional info:
Can you please test if [1] works on your machine? Thanks. [1] https://gerrit.ovirt.org/77344
Created attachment 1282304 [details] build.log The problem is still there. Tested cherry-picked on top of af407e0d0ff511c4. Build output attached.
Can you please try again, with your isort patched by (a variation of, if needed) this patch: --- /foo/git/isort/isort/settings.py 2016-05-16 09:31:31.000000000 +0300 +++ settings.py 2017-05-25 22:51:21.984929217 +0300 @@ -134,8 +134,11 @@ if editor_config_file and os.path.exists(editor_config_file): _update_with_config_file(editor_config_file, sections, computed_settings) +import traceback def _update_with_config_file(file_path, sections, computed_settings): + print("_update_with_config_file: file_path %s" % file_path) + traceback.print_stack() settings = _get_config_data(file_path, sections).copy() if not settings: return If this still does not work, I am afraid we'll simply have to give up and add a flag to activate the isort check and pass it from the automation scripts. That's a bit ugly imo, though.
Created attachment 1282542 [details] patched.zip still failing cherry-picked on to of 93e0fc7 patched file and compilation log attached
Can you please try passing ISORT=non-existant to make, as a workaround? Also, can you please patch your isort with the following patch: diff -urb /foo/git/isort/isort/isort.py ./isort.py --- /foo/git/isort/isort/isort.py 2016-05-16 09:31:31.000000000 +0300 +++ ./isort.py 2017-05-28 12:19:53.626832684 +0300 @@ -133,6 +133,7 @@ for section in itertools.chain(self.sections, self.config['forced_separate']): self.imports[section] = {'straight': set(), 'from': {}} + print('sections: %s' % ', '.join(self.sections)) self.index = 0 self.import_index = -1 self._first_comment_index_start = -1 diff -urb /foo/git/isort/isort/settings.py ./settings.py --- /foo/git/isort/isort/settings.py 2016-05-16 09:31:31.000000000 +0300 +++ ./settings.py 2017-05-28 16:56:08.900231603 +0300 @@ -109,8 +109,12 @@ @lru_cache() def from_path(path): computed_settings = default.copy() + print("from_path path %s" % path) + print("from_path 1") _update_settings_with_config(path, '.editorconfig', '~/.editorconfig', ('*', '*.py', '**.py'), computed_settings) + print("from_path 2") _update_settings_with_config(path, '.isort.cfg', '~/.isort.cfg', ('settings', 'isort'), computed_settings) + print("from_path 3") _update_settings_with_config(path, 'setup.cfg', None, ('isort', ), computed_settings) return computed_settings @@ -134,8 +138,11 @@ if editor_config_file and os.path.exists(editor_config_file): _update_with_config_file(editor_config_file, sections, computed_settings) +import traceback def _update_with_config_file(file_path, sections, computed_settings): + print("_update_with_config_file: file_path %s" % file_path) + traceback.print_stack() settings = _get_config_data(file_path, sections).copy() if not settings: return @@ -171,6 +178,7 @@ computed_settings[access_key] = list(_as_list(value)) else: computed_settings[access_key] = existing_value_type(value) + print("_update_with_config_file: sections %s" % ', '.join(computed_settings['sections']))
Hi Didi, I don't have capacity for that. I removed isort a need to continue on my tasks.
(In reply to jniederm from comment #6) > Hi Didi, I don't have capacity for that. I removed isort a need to continue > on my tasks. Very well. Thanks for the time you did spend on this! It seems to me like a bug in isort, or at least related to a bug in isort. But it's a bit hard to know as I didn't manage to reproduce it. So closing for now. Whoever else that stumbles upon this bug and considers reopening: 1. I think that start of comment 5, 'passing ISORT=non-existant to make', works. 2. We can still make this disabled by default, if there are many people affected and annoyed. 3. A real solution is finding the root cause/bug that makes isort behave differently between the CI system and your dev machine, and fix it. For the record: The bug seems to be that something causes isort to revert to its default sections (and specifically their order), ignoring the ones in the .isort.cfg in the git repo. The patch of Ido [1], to pass '-sp .isort.cfg', causes isort to _not_ read it - at least for me, verified by the patch in comment 3. One might claim this is a bug in isort, but not sure it's the bug we run into here. My patch [2], which is very similar to Ido's but passes the full path, still didn't work for jniederm, see comment 4. The other patch of Ido [3] to "fix" the code to work with [1], is the opposite of what we want - we want to make isort use the sections that we define, in the order we define - not its default. I am pretty certain that if we go with [1], effectively giving up on our custom isort conf and using its default, we'd need to make [3] much much larger. [1] https://gerrit.ovirt.org/77328 [2] https://gerrit.ovirt.org/77344 [3] https://gerrit.ovirt.org/76925
Managed to reproduce, with the help of msivak. It fails as described above if you do not have 'pytest' installed. If you do, it works. I guess that we install pytest in our CI slaves (as a dep?) so are not affected there.
Martin, can you verify that this patch solves the issue for you? Thanks. https://gerrit.ovirt.org/79592
For reference, I used the following patch against isort to debug it: commit 30a6a54509efcd867f8323ef05b093277b3e259c Author: Yedidyah Bar David <didi> AuthorDate: Wed Jul 19 12:01:10 2017 +0300 Commit: Yedidyah Bar David <didi> CommitDate: Wed Jul 19 12:01:17 2017 +0300 debug place_module diff --git a/isort/isort.py b/isort/isort.py index 70954b0..154d996 100644 --- a/isort/isort.py +++ b/isort/isort.py @@ -802,6 +802,7 @@ class SortImports(object): if import_type == "from": import_from = imports.pop(0) placed_module = self.place_module(import_from) + print("from-type place_module for %s returned %s" % (import_from, placed_module)) if placed_module == '': print( "WARNING: could not place module {0} of line {1} --" @@ -852,6 +853,7 @@ class SortImports(object): if self.index - 1 == self.import_index: self.import_index -= len(self.comments['above']['straight'].get(module, [])) placed_module = self.place_module(module) + print("else-type place_module for %s returned %s" % (module, placed_module)) if placed_module == '': print( "WARNING: could not place module {0} of line {1} --"
Pushed a variation to isort: https://github.com/timothycrosley/isort/pull/578
Yes, the fix resolves the issue for me. Thanks
Created attachment 1316457 [details] buildlog2 I can't compile current master (commit 7914051ad5) because this problem. Build log attached. Prefixing the build command by 'ISORT=non-existant ' didn't help nor did installation of pytest: $ pip list --format legacy | grep -i pytest pytest (3.2.1) Setting back to Assigned.
Target release should be placed once a package build is known to fix a issue. Since this bug is not modified, the target version has been reset. Please use target milestone to plan a fix for a oVirt release.
Pushed [1], which is just a guess, based on your log. Can you please try it? Alternatively, if it does not work, can you please try patching isort with the proposed pull request [2] and then share the output of: isort -vb --diff packaging/setup/tests/ovirt_engine_setup/engine_common/test_database.py [1] https://gerrit.ovirt.org/80882 [2] https://github.com/timothycrosley/isort/pull/578
Hi Didi, patch [1] from comment 15 fixed the problem for me. Thank you.
CodeChange - thus nothing for QE.
Moving to VERIFIED based on comment 16.
This bugzilla is included in oVirt 4.2.0 release, published on Dec 20th 2017. Since the problem described in this bug report should be resolved in oVirt 4.2.0 release, published on Dec 20th 2017, it has been closed with a resolution of CURRENT RELEASE. If the solution does not work for you, please open a new bug report.