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

Build: allow multi-line commands on build.commands #10334

Merged
merged 2 commits into from
May 23, 2023

Conversation

humitos
Copy link
Member

@humitos humitos commented May 22, 2023

This is another attempt to fix this issue. It follows the suggestion posted by @bensze01 in #10103. It looks simple and I think it makes sense to give it a try since it doesn't seem to be harmful.

I tested it locally with the branch
https://github.com/readthedocs/test-builds/tree/build-commands-multiline and it worked fine. Note that without this PR the branch linked fails.

Related #10133
Related #10119
Related #10206

External PRs that should be solved by this one:

Closes #10103
Closes #10208

This is another attempt to fix this issue. It follows the suggestion posted by
@bensze01 in #10103. It
looks simple and I think it makes sense to give it a try since it doesn't seem
to be harmful.

I tested it locally with the branch
https://github.com/readthedocs/test-builds/tree/build-commands-multiline and it
worked fine. Note that without this PR the branch linked fails.

Related #10133
Related #10119
Related #10206

External PRs that should be solved by this one:
* pypi/warehouse#12953
* pypi/warehouse#12953

Closes #10103
Closes #10208
@humitos humitos requested a review from ericholscher May 22, 2023 11:52
@humitos humitos requested a review from a team as a code owner May 22, 2023 11:52
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This seems... too simple. Do we know why this works? Agreed it seems worth trying if it solves the issue, but I feel like we probably want some kind of more robust solution, but baby steps :)

@bensze01
Copy link

@ericholscher It works because the PATH variable is always implicitly exported by the shell.

so PATH=foo command, PATH=foo; command and PATH=foo\ncommand produce identical results.

@bensze01
Copy link

To quote the POSIX spec:

2.5.3 Shell Variables

Variables shall be initialized from the environment (as defined by XBD Environment Variables and the exec function in the System Interfaces volume of POSIX.1-2017) and can be given new values with variable assignment commands. If a variable is initialized from the environment, it shall be marked for export immediately; see the export special built-in.

@ericholscher
Copy link
Member

@bensze01 Thanks for the explanation 💯

@humitos humitos enabled auto-merge (squash) May 23, 2023 03:58
@humitos humitos merged commit 85b7b56 into main May 23, 2023
@humitos humitos deleted the humitos/build-commands-multiline branch May 23, 2023 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Build: build.jobs.post_build produces mysterious shell syntax errors
3 participants