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

Fix Nailgun failure when the port is not specified #7878

Merged
merged 2 commits into from Jun 11, 2019

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Jun 10, 2019

Downstream Twitter tests were failing to compile certain targets with the error message:

zinc[zinc-java](path/to/target:target) failed: an integer is required (got type NoneType)

This happens when ProcessManager.socket returns None, which is an expected return value:

@property
def socket(self):
"""The running processes socket/port information (or None)."""
return self._socket or self.read_metadata_by_name(self._name, 'socket', self._socket_type)

This ends up resulting in the nailgun code trying to use None for the port, which is invalid and causes the error.

Instead, we use Pants' idiom of setting the default to None and in the body of the constructor then defaulting to the actual default value we care about.

@Eric-Arellano Eric-Arellano added this to the 1.17.x milestone Jun 10, 2019
@Eric-Arellano Eric-Arellano changed the title Fix Nailgun failure when the port cannot be read Fix Nailgun failure when the port is not specified Jun 10, 2019
Copy link
Contributor

@ity ity left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

The commit description seems over the top to me. This is all just the common pattern of using meaningful default param values instead of None plus check in constructor. That central lesson and fix is pretty well obscured by a wall of text.

src/python/pants/java/nailgun_executor.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

The commit description seems over the top to me.

Agreed, and very good feedback. Because I hadn't yet run the fix through Twitter tests—and it took a while to figure out how all the pieces fit together—I was retracing my steps as an exercise in reasoning about the correctness of the solution. While useful to me, doesn't mean it's helpful in the PR description. I shortened it quite a bit.

Thanks for the review both!

src/python/pants/java/nailgun_executor.py Outdated Show resolved Hide resolved
@Eric-Arellano Eric-Arellano merged commit 5758cce into pantsbuild:master Jun 11, 2019
@Eric-Arellano Eric-Arellano deleted the fix-nailgun-none-error branch June 11, 2019 00:35
Eric-Arellano added a commit that referenced this pull request Jun 11, 2019
Downstream Twitter tests were failing to compile certain targets with the error message:

```
zinc[zinc-java](path/to/target:target) failed: an integer is required (got type NoneType)
```

This happens when `ProcessManager.socket` returns `None`, which is an expected return value:

https://github.com/pantsbuild/pants/blob/e727747e036584301fb4dac5203597fd1066d6f7/src/python/pants/pantsd/process_manager.py#L306-L309

This ends up resulting in the nailgun code trying to use `None` for the `port`, which is invalid and causes the error.

Instead, we use Pants' idiom of setting the default to `None` and in the body of the constructor then defaulting to the actual default value we care about.
@Eric-Arellano Eric-Arellano removed this from the 1.17.x milestone Jun 13, 2019
cattibrie pushed a commit to cattibrie/pants that referenced this pull request Jun 19, 2019
Downstream Twitter tests were failing to compile certain targets with the error message:

```
zinc[zinc-java](path/to/target:target) failed: an integer is required (got type NoneType)
```

This happens when `ProcessManager.socket` returns `None`, which is an expected return value:

https://github.com/pantsbuild/pants/blob/e727747e036584301fb4dac5203597fd1066d6f7/src/python/pants/pantsd/process_manager.py#L306-L309

This ends up resulting in the nailgun code trying to use `None` for the `port`, which is invalid and causes the error.

Instead, we use Pants' idiom of setting the default to `None` and in the body of the constructor then defaulting to the actual default value we care about.
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

3 participants