Skip to content

Remove deprecated flags args #3385

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

Merged
merged 1 commit into from
Dec 15, 2016
Merged

Conversation

theotherjimmy
Copy link
Contributor

Status

READY

Migrations

YES

Anyone using the --cflags, --cppflags, or --ldflags arguments are advised that they have no affect, and should stop passing them. Please migrate any flag shenanigans to using --profile

TODOs

  • CI

@theotherjimmy
Copy link
Contributor Author

/morph test-nightly

@theotherjimmy
Copy link
Contributor Author

Reviewers: Could you verify that removing these flags will not break any of your CI, tests, or other stuff?

@mbed-bot
Copy link

mbed-bot commented Dec 8, 2016

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1220

All builds and test passed!

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 8, 2016

Anyone using the --cflags, --cppflags, or --ldflags arguments are advised that they have no affect, and should stop passing them. Please migrate any flag shenanigans to using --profile

How are they advised? I assume these args are removed, and a user needs to search for file changes and find out why they were removed and what to use instead. Shouldn't we accept these arguments and print a warning Deprecated argument that has currently no effect, use --profile instead. for some specific time. Again until the next major would be probably a good path forward? (This depends however when we integrate this, if for any next 5.3.x patch? Or 5.4? I would assume we want to get this in asap)

Nit: This could be helpful to a reader of the commit message (once I find out that this broke my build, I would find this commit and try to get a reason why and how to use new options). I would provide more information to the commit message (migration path - use --profile debug/default/small instead + links -> references to the docs how to use profiles). From one commit I would be able to get all the information I need to migrate and update my codebase.

@adbridge
Copy link
Contributor

adbridge commented Dec 8, 2016

Agree with @0xc0170 we've seen a number of changes like this recently which are not obvious about how to go about fixing. I think the suggestion about adding a deprecation message with meaningful information and links to any documentation is definitely a good suggestion!

@theotherjimmy
Copy link
Contributor Author

@0xc0170 Thanks for the feedback. I'll update the commit message to be more specific.

Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

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

Won't hurt the mbed OS CI, LGTM

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2016

/morph test

@mbed-bot
Copy link

mbed-bot commented Dec 9, 2016

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1240

All builds and test passed!

@bridadan
Copy link
Contributor

bridadan commented Dec 9, 2016

@theotherjimmy One thing that's still not clear to me is if a user supplies any one of the 3 removed arguments, will they be greeted with an argparse error? @0xc0170 and @adbridge brought up a good point about lots of changing parameters recenetly, -o to --profile being the biggest one. While that was the right change in my opinion, I think we could have removed some headache by providing a deprecation message pointing the users at --profile. Can we do the same here, at least for the next few releases?

The tools will no longer accept `--cflags`, `--cppflags`, or
`--ldflags`. Instead, the ability to modify these flags is
provided by the `--profile` argument. Documentation for the
`--profile` argument may be found in
docs/Toolchain_Profiles.md
@theotherjimmy
Copy link
Contributor Author

@0xc0170 @adbridge @bridadan I changed the implementation to say Deprecated everywhere when trying to use these args. I will submit another PR to remove them completely for the 5.4 release.

Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

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

Nice change, LGTM

@bridadan
Copy link
Contributor

bridadan commented Dec 9, 2016

Marking as needs: review so @0xc0170 and/or @adbridge can take a look at the new changes.

@adbridge
Copy link
Contributor

@mazimkhan how do we re-run uvisor test on this?

@adbridge
Copy link
Contributor

/morph test-nightly

@adbridge
Copy link
Contributor

/morph export-build

@adbridge
Copy link
Contributor

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@mazimkhan
Copy link

retest uvisor

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1253

Test Prep failed!

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 53

Exporter Build failed!

@theotherjimmy
Copy link
Contributor Author

theotherjimmy commented Dec 12, 2016

nightly failure is a synchronization issue on windows in preparation (two threads trying to access the same file from two contexts).

@theotherjimmy
Copy link
Contributor Author

export build seems to be the duplicate i2c_api.c files issue.

@bridadan
Copy link
Contributor

@theotherjimmy Can you comment a bit more on the nightly failure? I admit I haven't seen that issue before. I'll go ahead and rerun the nightly.

/morph test-nightly

@theotherjimmy
Copy link
Contributor Author

/morph export-build

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1255

All builds and test passed!

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 54

Exporter Build failed!

@theotherjimmy
Copy link
Contributor Author

I still don't think that the intermittent uvision failures are related. They seem to affect a random target every time and only one object file each time.

@bridadan
Copy link
Contributor

/morph export-build

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 62

All exports and builds passed!

@theotherjimmy
Copy link
Contributor Author

/morph export-build

@theotherjimmy
Copy link
Contributor Author

Dang it. These pages need to stop being cached and not updating.

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 64

All exports and builds passed!

@theotherjimmy
Copy link
Contributor Author

@0xc0170 @sg- merge please?

@sg- sg- merged commit 18f31b7 into ARMmbed:master Dec 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants