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

Remove duplicate build preset declarations #248

Merged
merged 1 commit into from
Dec 10, 2015
Merged

Remove duplicate build preset declarations #248

merged 1 commit into from
Dec 10, 2015

Conversation

RLovelett
Copy link
Contributor

Python 3's configparser does not parse in the same way that Python 2's did. Specifically, Python 3's does not allow duplicate sections (or in Swift parlance presets).

Python 3 does provide a "strict" property that can be sent to the constructor to return to the behavior of Python 2 (e.g., config = ConfigParser.SafeConfigParser(substitutions, allow_no_value=True, strict=False)). However, this provides it's own set of complications in that the Python 2 version of the constructor does not.

Therefore, instead of having a conditional in the runtime to detect which version of Python and build the correct initializer this patch normalizes the presets configuration file such that there are no more duplicates. This should work in both Python 2 and 3.

Also worth noting: this patch is insufficient by itself to make the build scripts work with Python 3. This patch is written with the "Incremental Development" theory in mind and in preparation for other patches currently in progress, (i.e., #90).

@benlangmuir
Copy link
Contributor

Why did you inline the settings into the presets? For example, you added settings to buildbot,tools=RA,stdlib=DA that looks identical to what's already there because of mixin_buildbot_tools_RA_stdlib_DA.

@RLovelett
Copy link
Contributor Author

@benlangmuir I guess I don't fully understand the question. So instead I'll explain what I did and then hopefully that makes sense.

If you look at build-presets.ini on master the definition for buildbot,tools=RA,stdlib=DA occurs twice. Once on line 56 and then again on line 179. The Python 3 configparser does not support defining this twice (unless you disable strict mode).

To resolve this I moved the definition made on line 56 down to line 179. It was 99% a cut-and-paste. The only thing that was not cut-and-paste was merging the mixin-preset definition that was made for each individual preset defintion.

I then repeated that same process for preset: buildbot,tools=RA,stdlib=RD and preset: buildbot,tools=RA,stdlib=RDA.

@RLovelett
Copy link
Contributor Author

@benlangmuir I guess now I see what your saying. The definition of preset: buildbot,tools=RA,stdlib=DA on line 56-68 is exactly the same as the definition of mixin_buildbot_tools_RA_stdlib_DA on line 164-176.

So why not just delete the definition of preset: buildbot,tools=RA,stdlib=DA on line 56-68? Frankly I hadn't noticed it until you asked this question.

That now seems like a better strategy with less churn. I'll update accordingly.

@benlangmuir
Copy link
Contributor

EDIT: didn't see your second comment, which is exactly what I was trying to say here 😺
Okay, that's what I thought happened. My point is that we don't need to merge the first definition into the second one, we can just delete the first definition. The two definitions were already equivalent because the first instance of buildbot,tools=RA,stdlib=DA is identical to mixin_buildbot_tools_RA_stdlib_DA which is used by buildbot,tools=RA,stdlib=DA. Does that make sense?

@RLovelett
Copy link
Contributor Author

Yeah it does. We are on the same page now. I'll update that.

As noted by suggestions by @benlangmuir the presets are actually
duplicated. Therefore, they can safely be removed and not "merged".

Python 3's
[configparser](https://docs.python.org/3/library/configparser.html) does
not parse in the same way that Python 2's did. Specifically, Python 3's
does not allow duplicate sections (or in Swift parlance presets).

Python 3 does provide a "strict" property that can be sent to the
constructor to return to the behavior of Python 2 (e.g.,  `config =
ConfigParser.SafeConfigParser(substitutions, allow_no_value=True,
strict=False)`). However, this provides it's own set of complications in
that the Python 2 version of the constructor does not.

Therefore, instead of having a conditional in the runtime to detect
which version of Python and build the correct initializer this patch
normalizes the presets configuration file such that there are no more
duplicates. This should work in both Python 2 and 3.

Also worth noting: this patch is insufficient by itself to make the
build scripts work with Python 3. This patch is written with the
"Incremental Development" theory in mind and in preparation for other
patches currently in progress.
@RLovelett
Copy link
Contributor Author

I've rebased this on the latest master (ab29adf) and have incorporated @benlangmuir's suggestion.

@gribozavr
Copy link
Contributor

@RLovelett Did you test any of the affected presets?

@RLovelett
Copy link
Contributor Author

I've run a build with swift/utils/build-script --preset=buildbot_linux_1510 and it was able to configure and begin running successfully.

EDIT:
I guess I should point out that it has yet to fully compile. However, it got passed the initial build config parsing part and actually started compiling. I just am having a problem where the linker is not finding -lpthread -ldl but I don't think that is related to this patch because I get it with-or-without this patch being applied.

@gribozavr
Copy link
Contributor

This LGTM.

gribozavr added a commit that referenced this pull request Dec 10, 2015
Remove duplicate build preset declarations
@gribozavr gribozavr merged commit cb1aa88 into swiftlang:master Dec 10, 2015
@RLovelett RLovelett deleted the python3-build-presets branch December 10, 2015 20:48
slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
clarify config of pthread_workqueue vs. internal_workqueue
slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
clarify config of pthread_workqueue vs. internal_workqueue

Signed-off-by: Daniel A. Steffen <[email protected]>
kateinoigakukun added a commit to kateinoigakukun/swift that referenced this pull request Mar 1, 2020
…ed-tests

[WASM] Disable unsupported test suite on WASM
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
Update R.swift hash to build against 4.2
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.

3 participants