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: fix ENAMETOOLONG for too big commit messages #263

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ChristianUlbrich
Copy link

This PR allows running semantic-release to reliably commit arbitrary big commit messages. The old way of adding a commit message was by passing it simply along as a command line argument to git commit -m ${message} this will easily break for huge commit messages, depending on the platform:

  • Windows seems to limit the total length of a program execution "string" (and thus a execa() call to roughly 8 KB
  • and there are limits for the total length per passed argument (on my system 128KB)

In practice one can easily hit those limits by having a moderate number of changes during releases, because upstream in semantic-release for a default setup of semantic-release/changelog, semantic-release/git the generated changelog is added as a commit message.

The fix is, that the message is not passed as an argument, but a temporary file containing the message will be created and the file will be passed instead.

There are now some additional implications; whenever something goes wrong, there is the possibility that temporary files are not deleted, however as os.tmpdir() is used, this is something that the OS should deal with. Furthermore this now will break, if os.tmpdir() is not writeable by semantic-release. However I think it is a fair assumption, that os.tmpdir()is writeable - although an additional check could be added.

Whether this is a breaking change, this is left as an exercise for the astute reader something to be answerer by the maintainers. I'd say it is not, because it is behaving as intended. However this might break some pipelines, that have not os.tmpdir() writeable. I tested it on an Azure DevOps Windows hosted agent and I am pretty sure™ that in a sane default™ setup of build agents this should not pose any problems.

This is somehow related to: #91 (although I think the actual course for the problem there was the amout of changed files); at least it is giving the same error message.

There are further places in the code, where seemingly unrestricted data is passed via execa() that is suffering from the same problems (adding a lot of files via add), however fixing this is not so easy, because git add does not support adding files from a "list file". Possible solutions would be a chunked adding (splitting the files by "length" and thus multiple git add calls) - however this has further implications. I'd rather fix those problems in another PR if the maintainers are fine with this approach (or suggesting sth. better).

@ChristianUlbrich
Copy link
Author

@pvdlg Ping, is there anything not clear? This is breaking our build currently and while using the fix directly is really easy (because all plugins are old-school w/o any bundling yay!), our team lead is somewhat hesitant using private fixes from GitHub. :)

@ChristianUlbrich
Copy link
Author

@travi BUMP. This is still a problem... Luckily the one year old fix, still happily merges. :)

@travi
Copy link
Member

travi commented Aug 2, 2024

i'm sorry that i've been silent on this thread for so long. it can be tough to keep up with all of our priorities and this one has, unfortunately, fallen through the cracks so far. i have been aware of this request and have had it on my radar for quite a while, but have not been able to give it priority.

i recently had a conversation about this situation, so i want to capture and share some details that i think are important to consider for this case:

  • this issue is filed in the repo for the git plugin, so that already suggests this, but i want to call it out specifically: this only happens when making a commit as part of your release process. there are valid situations when this is necessary, but we do actively discourage making commits, both in the readme for the project and in our FAQ. we still support making commits, but we do prioritize this approach a bit lower than other areas since it is a practice we discourage.
  • as mentioned in An error occurred while running semantic-release: { Error: spawn ENAMETOOLONG #91 (comment), this is far more likely to happen on windows. we do support windows, but since semantic-release is intended to be run from CI and CI services typically make it simpler to run workflows on linux, our usage on windows is far lower than on linux. if possible, we recommend folks avoid windows for release workflows, but we understand that some situations require that context
  • this is basically a result of really long release notes being included in the commit body. situations that result in such long release notes should be rare if following the recommended workflows that leverage the practice of continuous integration. having long release notes usually means lots of commits being released as a big batch. this can happen when adding semantic-release to an existing project without the necessary tags in place to inform semantic-release how far it should look back in history, but it is a more regular concern for teams that are following gitflow and have lots of changes in a batch when promoting to a stable release
  • semantic-release officially does not support gitflow, but we know that many of our users leverage our pre-release workflow in order to simulate a workflow that is close. pre-releases are intended for experimental changes or changes that need iteration with consumers to define an acceptable public api for a new feature. pre-releases are not intended to support a testing workflow. we encourage teams to find ways to enable verification of known behaviors that do not require publishing WIP releases to confirm that the project is working as defined

hopefully some of that context is helpful for some folks that land here to understand some of the contributors to the situation and potentially find some adjustments that could be made to avoid this situation.

all of that said, part of what has delayed my acceptance of this proposed solution is that it feels like an overly complex solution to a problem that should be very rare under normal circumstances. the "very rare under normal circumstances" makes it hard to prioritize in the first place, but to add the need to write a temp file, which potentially needs additional system access, only to read it back in for this rare case makes this hard to accept and maintain over the long term. i'm also very hesitant to do anything along the lines of truncating the content because i think that presents the worst of both worlds. we would only partially be enabling the behavior of including the release notes in the commit.

i admit, i'm unsure about why we include the release notes in the commit body in the first place. i can understand some folks might value that information being directly in the commit history, but i wonder if that is something many folks give much attention to. that behavior existed before i became a maintainer. i honestly wonder if the simpler solution would be to simply not include that information in the commit. we already support various ways to capture release notes in more visible locations, so maybe it doesnt need to be in the commit. that would be a breaking change, but maybe a justified one.

as highlighted in #91 (comment), it is already possible to opt-out of this default behavior. maybe the best course of action would be to flip this to an opt-in instead? i'm curious if folks have thoughts about this option

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.

2 participants